20b3703a42f238c6a3a717f6f9ec99301feca958
28
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xxxigm
|
20b3703a42 |
fix(conversation-loop): tailor length-continuation prompt for partial stream
The length-continue path's user-facing vprint and continuation prompt both told the model "your response was truncated by the output length limit." That's a lie when the stub came from a partial-stream network error (issue #30963) — and a lie the model can detect, leading to "I wasn't truncated, I'm done" no-op responses that defeat the continuation entirely. Detect the partial-stream-stub via response.id and swap in: - vprint: "Stream interrupted by network error (finish_reason='length' on partial-stream-stub)" - prompt: "[System: The previous response was cut off by a network error mid-stream. Continue exactly where you left off. Do not restart or repeat prior text. Finish the answer directly.]" Real length truncations still see the original "truncated by output length limit" prompt — the model needs to know which class of failure it's recovering from. Same length_continue_retries=3 budget, truncated_response_parts merging, and final-response stitching infrastructure on both branches. Refs: NousResearch/hermes-agent#30963 |
||
|
xxxigm
|
9140be7c22 |
fix(streaming): emit finish_reason=length on text-only partial-stream stub
When the API connection drops mid-stream after text deltas have already been delivered, chat_completion_helpers returned a stub response with finish_reason=stop. The conversation loop then classified the stub as a clean text completion (text_response(finish_reason=stop)) and exited with iteration budget remaining — even when the goal-judge verdict came back as "continue" milliseconds later (issue #30963). Switch the text-only partial-stream stub to finish_reason=length. The existing length-continuation path (length_continue_retries up to 3, "continue exactly where you left off" prompt, partial parts merged into final_response) then fires automatically: the partial assistant content is persisted, the model is asked to continue from the cut point, and the loop keeps making progress against the goal. The mid-tool-call branch keeps finish_reason=stop on purpose — its user-facing warning ("Ask me to retry if you want to continue") asks the user to drive the retry rather than auto-replaying a tool call with possible side effects. #5544's "no duplicate message" contract is preserved verbatim: the partial content is reused, never re-emitted as a fresh API call, so the user never sees two copies of the same delta. Refs: NousResearch/hermes-agent#30963 |
||
|
xxxigm
|
b5ea6a5c80 |
test(xai-oauth): regression coverage for the bad-credentials disambiguator (#29344)
Eleven new tests pinning the #29344 fix. Layout mirrors the existing "Fix D" entitlement section so the bad-credentials disambiguator sits alongside the entitlement-block tests it complements. Classifier-level coverage: * ``test_is_entitlement_failure_false_for_bad_credentials_wke_suffix`` — verbatim shape from the reporter's wire capture (``{code: 'caller does not have permission', error: 'OAuth2 access token could not be validated. [WKE=unauthenticated:bad-credentials]'}``) ↦ classifier must return False so the refresh path runs. * ``test_is_entitlement_failure_false_for_wke_suffix_in_normalized_shape`` — same body after ``_extract_api_error_context`` has rewritten it to ``{reason, message}``. The disambiguator must fire in BOTH shapes; without this guard the production call site at ``_recover_with_credential_pool`` (which goes through the normalised extractor) would still misclassify. * ``test_is_entitlement_failure_false_for_any_wke_unauthenticated_variant`` — parametrised forward-compat: ``bad-credentials``, ``expired-token``, ``revoked``, ``some-future-reason``. xAI documents the prefix as stable, the suffix after the colon as a reason code that can grow; every variant under ``unauthenticated:`` must route to refresh. * ``test_is_entitlement_failure_false_via_oauth2_validation_phrase_alone`` — belt-and-braces guard: if a future API revision drops the WKE suffix but keeps "OAuth2 access token could not be validated", we still classify correctly. * ``test_is_entitlement_failure_wke_signal_overrides_entitlement_keywords`` — defensive: if a body ever carries BOTH the WKE suffix and entitlement language, the WKE signal wins. Auth is recoverable; entitlement isn't, and a refreshed token will resurface the entitlement message on the next request. * ``test_is_entitlement_failure_case_insensitive_wke_match`` — pins that the classifier lowercases the haystack so a future xAI build that uppercases the prefix doesn't reintroduce the bug. Recovery-path coverage (end-to-end through ``_recover_with_credential_pool``): * ``test_recover_with_credential_pool_refreshes_on_xai_bad_credentials_403`` — the headline test the reporter requested: a bad-credentials 403 with the exact wire body must call ``try_refresh_current()`` exactly once and ``_swap_credential`` once. Pre-fix this returned ``(False, _)`` because the entitlement classifier over-matched and short-circuited the refresh path. * ``test_recover_with_credential_pool_still_blocks_real_entitlement`` — companion regression guard for #26847: a pure unsubscribed- account body (no WKE suffix, no OAuth2-validation phrase) must still surface as entitlement and skip refresh. The new disambiguator must not weaken the original loop-protection it was added to preserve. The scaffolding reuses ``_make_codex_agent``, ``_FakePool``, and the existing ``MagicMock`` patterns from the surrounding tests so the new section reads as a natural extension of "Fix D" rather than a separate test file. |
||
|
xxxigm
|
8b3cb930c9 |
fix(xai-oauth): honor [WKE=unauthenticated:...] disambiguator in entitlement classifier (#29344)
``_is_entitlement_failure`` over-matched on xAI 403s. xAI returns the
same permission-denied ``code`` text for two distinct conditions:
1. Unsubscribed account ("active Grok subscription. Manage at
https://grok.com" in the ``error`` field).
2. Stale OAuth access token ("OAuth2 access token could not be
validated. [WKE=unauthenticated:bad-credentials]" in the ``error``
field).
The classifier's "does not have permission + grok" substring heuristic
treated both identically, so the credential-pool refresh path was
short-circuited for case (2) — long-running TUI sessions stuck on a
stale OAuth token surfaced a non-retryable client error and the user
had to exit + reopen the TUI to recover (the startup-resolve path
bypasses the classifier entirely, which is why bridge adapters with
proactive refresh cadences didn't see this in practice).
This patch adopts the reporter's recommended fix (option 1, tightest):
honor xAI's explicit ``[WKE=unauthenticated:...]`` suffix and the
``OAuth2 access token could not be validated`` phrasing as
authoritative "this is auth, not entitlement" signals. When either
appears anywhere in the body's text fields, the classifier returns
False eagerly — *before* the entitlement keyword checks run — so the
refresh-on-401 path takes over and the existing loop-protection still
guards against runaway refresh storms if the refresh itself fails.
Two small adjustments fall out of this:
* The haystack now also covers ``code`` and ``error`` keys directly,
not just the ``message``/``reason`` shape ``_extract_api_error_context``
produces. Real runtime paths use the normalised shape, but the test
suite and any future call sites that pass raw bodies get the same
treatment. Backwards compatible: missing keys default to empty
strings, the haystack still skips when everything is blank.
* Both disambiguator checks fire BEFORE the entitlement keyword
checks. If a future xAI body somehow lands with both an entitlement
message AND the WKE suffix, the WKE suffix wins (correct — auth is
recoverable; entitlement is not, and a refreshed token will surface
the entitlement message on the next request anyway).
Existing tests (``test_is_entitlement_failure_matches_real_xai_bodies``,
``test_is_entitlement_failure_false_for_unrelated_auth_errors``,
``test_recover_with_credential_pool_skips_refresh_on_entitlement_403``,
``test_recover_with_credential_pool_still_refreshes_genuine_auth_failure``)
continue to pass unchanged — the unsubscribed-account path, the
generic auth-error path, and the refresh-on-401 path are all left
intact.
|
||
|
xxxigm
|
5b6f0b695b |
test(tls-fd-recycle): pin shutdown-only + thread-aware close contract (#29507)
Ten regressions across both prongs of the #29507 fix, organised so each test names exactly which way the bug could come back: Prong 1 — ``force_close_tcp_sockets``: * ``shutdown_only_no_close`` is the smoking-gun assertion. If a future refactor adds back ``sock.close()`` to this helper, the FD-recycling race that wrote TLS bytes on top of ``kanban.db`` is back, and this trips. * ``uses_shut_rdwr`` pins that both halves are shut down (a half-close wouldn't unblock a worker stuck in ``recv``). * ``swallows_oserror_on_shutdown`` covers the already-shutdown case. * ``handles_multiple_pool_entries`` walks all pool connections. Prong 2 — thread-aware ``_close_request_client_once``: * ``stranger_thread_aborts_only_no_close`` simulates the asyncio_0 → Thread-1616 interrupt path: stranger drives abort, holder stays populated for the worker's eventual finally. * ``owner_thread_pops_and_full_close`` is the worker-thread path: pops + full close. * ``stranger_then_owner_close_sequence_runs_full_close_exactly_once`` replays the reporter's exact timeline at object level: abort runs once, full close runs once, holder ends empty. Agent surface: * ``_abort_request_openai_client_does_not_call_client_close`` pins that the new entrypoint shuts sockets and emits the ``deferred_close=stranger_thread`` marker but never calls ``client.close()``. * ``_abort_request_openai_client_null_client_is_noop`` defensive. End-to-end: * ``fd_recycle_window_closed_by_shutdown_only`` reproduces the race at object level — runs the abort path from a stranger thread and asserts that no ``close()`` ever fires, so the kernel can never recycle the FD under the owner's still-active reference. |
||
|
xxxigm
|
30c22f1158 |
fix(api-call): defer client.close() to owning worker thread on interrupt (#29507)
Layer-2 defense for the FD-recycling race: even with ``force_close_tcp_sockets`` reduced to shutdown-only, the followup ``client.close()`` in ``_close_openai_client`` still walks the httpx pool and closes sockets — and if called from a stranger thread (the interrupt-check loop, the stale-call detector) it has the same FD-recycling exposure that wrote a TLS record on top of ``kanban.db``. Stamp the request_client_holder with the owning thread's ident at ``_set_request_client`` time. In ``_close_request_client_once``: * Owning thread (the worker's ``finally``) → pop + ``client.close()`` via ``_close_request_openai_client``, exactly as before. * Stranger thread → ``_abort_request_openai_client`` (new): only ``shutdown(SHUT_RDWR)`` the pool sockets and log a deferred-close marker. The holder stays populated so the worker's eventual ``finally`` performs the real close from its own thread context, where the FD release races nothing. Applied symmetrically to both the non-streaming ``interruptible_api_call`` and the streaming variant — both routinely get hit by stranger-thread interrupts. The log field ``tcp_force_closed=N`` keeps its existing shape; the new abort path adds ``deferred_close=stranger_thread`` so production triage can distinguish the two close kinds. |
||
|
xxxigm
|
e2a7d73a66 |
fix(force_close_tcp_sockets): shutdown only, do not release FD (#29507)
The helper used to call ``socket.shutdown(SHUT_RDWR)`` followed by ``socket.close()`` to drop CLOSE-WAIT entries immediately. On its own ``shutdown()`` is safe from any thread — it only sends FIN and breaks pending ``recv``/``send`` — but ``close()`` releases the FD integer to the kernel. When the helper runs on a stranger thread (the interrupt loop, the stale-call detector) the FD release races the owning httpx worker thread that still has the same integer cached inside the SSL BIO. The kernel then recycles that integer to the next ``open()`` call — in production, kanban dispatcher's ``kanban.db`` — and the worker's delayed TLS flush writes a 24-byte TLS application-data record on top of the SQLite header. Restrict the helper to ``shutdown(SHUT_RDWR)`` only. The owning httpx worker's own unwind will close the underlying socket via the same Python ``socket.socket`` object, which atomically swaps ``_fd`` to -1 before issuing ``close(2)`` — no FD-aliasing window. The log field ``tcp_force_closed=N`` is kept (now counts shutdowns) so existing dashboards / log parsers keep working. |
||
|
xxxigm
|
6a2df9f451 |
docs(env): clarify HERMES_ENABLE_PROJECT_PLUGINS contract (#29156)
The reference entry now documents the truthy set (``1`` / ``true`` / ``yes`` / ``on``) explicitly, matches the falsy half (``0`` / ``false`` / ``no`` / ``off`` / empty string) that the GHSA-5qr3-c538-wm9j fix re-aligned both the agent loader and the dashboard web server around, and points readers at the defence-in-depth rule that project plugins never have their Python ``api`` file auto-imported by the dashboard regardless of the env var. |
||
|
xxxigm
|
8bf99227f0 |
fix(plugins): block plugin-api path traversal + project RCE (#29156)
GHSA-5qr3-c538-wm9j — half two of the bypass chain.
``_mount_plugin_api_routes`` imports each dashboard plugin's
manifest ``api`` field as a Python module via
``importlib.util.spec_from_file_location`` — arbitrary code
execution by design. Two primitives in the surrounding code
turned that "by design" RCE into a usable attack:
1. Absolute paths in the manifest swallow the plugin directory.
``Path('safe/dashboard') / '/tmp/evil.py'`` resolves to
``/tmp/evil.py``, so a single manifest line
``{"api": "/tmp/payload.py"}`` was enough to redirect the
importer at any Python file on disk.
2. ``..`` traversal in the manifest climbs out of the dashboard
directory. ``Path('plugins/safe/dashboard') /
'../../../tmp/evil.py'`` lands in ``/tmp/evil.py`` after
``resolve()`` — the static-asset handler
(``serve_plugin_asset``) already defends against this via
``is_relative_to``; the api-mount path didn't.
Fix at three layers so a regression in any one can't re-open the
advisory:
* New ``_safe_plugin_api_relpath`` validator runs at *discovery*
time and stores only sanitised relative paths on the plugin
entry's ``_api_file`` field. Absolute paths, ``..`` traversal,
empty / non-string values, and paths that ``resolve()`` outside
the plugin's ``dashboard/`` directory are rejected with a
warning naming the plugin. ``has_api`` follows the sanitised
value so the dashboard frontend doesn't render a fake "Backend
API" badge for plugins whose api was scrubbed.
* ``_mount_plugin_api_routes`` re-validates the resolved path
against the live filesystem just before the import — defence in
depth in case ``_dir`` is tampered with post-cache or a future
caller bypasses the discovery-time validator.
* Project plugins (``source == "project"``) are refused outright
for backend import. ``./.hermes/plugins/`` ships with the CWD,
so any threat model that includes "user opens a malicious repo"
treats it as attacker-controlled; project plugins can still
extend the UI via static JS/CSS but their Python ``api`` is no
longer auto-imported. Combined with the truthy env-gate fix
from the previous commit, the original advisory chain now
fails at two distinct choke points.
|
||
|
xxxigm
|
da636e982b |
test(plugins): regression coverage for project-plugin RCE chain (#29156)
35 new tests across 5 classes covering every layer of the GHSA-5qr3-c538-wm9j defence. Each class corresponds to one chokepoint so a regression in any single layer is caught by the named class: * ``TestProjectPluginsEnvGate`` (13 cases) — parametrised over both the documented truthy values (``1`` / ``true`` / ``yes`` / ``on`` + uppercase variants) and the previously-bypassing falsy strings (``0`` / ``false`` / ``no`` / ``off`` / ``""`` / ``False``). The falsy half is the direct env-bypass repro: pre-fix any non-empty string enabled the project source. * ``TestApiPathSanitizer`` (16 cases) — unit-level coverage of the new ``_safe_plugin_api_relpath`` helper. Absolute paths (``/etc/passwd``, ``/tmp/payload.py``, ``/usr/bin/python``), ``..``-traversal payloads (including nested ``subdir/../../..``), and non-string / empty / whitespace-only values must all return ``None``. Safe relative paths (``api.py``, ``backend/routes.py``) round-trip unchanged so legitimate plugins keep working. * ``TestDiscoveryScrubsApiField`` (3 cases) — end-to-end through ``_discover_dashboard_plugins`` with a real manifest on disk. Verifies that the cached plugin entry's ``_api_file`` is scrubbed *at discovery time* (``None`` + ``has_api: False``) so any downstream consumer can't be tricked into re-deriving the unsafe path from cache. * ``TestMountApiRoutesRefusesUntrusted`` (3 cases) — pokes synthetic plugin entries with each refusal vector directly into the cache and patches ``importlib.util.spec_from_file_location`` to assert it is *not* invoked for project-source / traversal payloads, and *is* invoked normally for bundled / user plugins. * ``TestEndToEndPocBlocked`` (1 case) — reproduces the original advisory PoC: operator sets ``HERMES_ENABLE_PROJECT_PLUGINS=0`` believing project plugins are off, attacker plants a manifest in CWD's ``.hermes/plugins/`` with ``api`` pointing at an absolute payload path. Asserts that the importer is never called against the payload path *and* that ``hermes_dashboard_plugin_evil`` is not in ``sys.modules`` after the mount routine runs. An autouse fixture busts ``_dashboard_plugins_cache`` before and after each test so the production cache (populated by the import-time ``_mount_plugin_api_routes()`` call) can't bleed in. All 12 pre-existing dashboard-plugin tests in ``test_web_server.py`` still pass unchanged. |
||
|
xxxigm
|
09f85f2cf7 |
fix(plugins): apply truthy env semantics to project-plugin gate (#29156)
GHSA-5qr3-c538-wm9j — half one of the bypass chain.
``_discover_dashboard_plugins`` opted into the untrusted ``./.hermes/
plugins/`` source via ``if os.environ.get("HERMES_ENABLE_PROJECT_
PLUGINS"):`` — which is True for any non-empty string. ``=0``,
``=false``, ``=no``, ``=off`` all return non-empty strings and so
*enabled* the project source even though every operator (and the
agent loader, ``hermes_cli/plugins.py`` line 815) reads those values
as "disabled". An attacker who can land a manifest under the CWD's
``.hermes/plugins/`` directory — a malicious cloned repo, a worktree
checked out from a forked PR, a CI runner workspace — was therefore
guaranteed to get their manifest discovered the moment the user ran
``hermes dashboard`` from that directory, regardless of whether the
user thought they had project plugins disabled.
Switch to the shared ``utils.env_var_enabled`` helper used by the
agent loader so the gate accepts the documented truthy set (``1`` /
``true`` / ``yes`` / ``on``, case-insensitive) and treats everything
else — including ``0`` / ``false`` / ``no`` — as off.
Half two (path-traversal + project-source ``api`` import) lands in
the next commit. Together they break the RCE chain at two distinct
choke points so a future regression in either one alone can't
re-open the advisory.
|
||
|
xxxigm
|
bec2250d2c |
test(computer_use): end-to-end regression for capture routing (#24015)
Add tests/tools/test_computer_use_capture_routing.py — 13 integration
tests that drive _capture_response end-to-end with deterministic stubs
for the routing helper, _run_async, vision_analyze_tool, and
get_hermes_dir, so the full code path is exercised without a live
cua-driver, real auxiliary client, or network access.
Coverage:
* TestCaptureResponseDefaultPath (3 cases)
- SOM PNG capture returns the legacy multimodal envelope when the
routing helper says 'native' (image/png MIME).
- Same path returns image/jpeg MIME for JPEG payloads (cua-driver
can return either).
- AX-only mode never even consults the routing helper because no
PNG is present.
* TestCaptureResponseRoutedToAuxVision (5 cases)
- SOM capture with routing on returns a JSON string with the
vision_analysis embedded, the AX/SOM index preserved, and NO
image_url parts. Verifies the aux call receives a path under
the configured cache and a prompt that grounds itself against
the AX summary.
- Temp screenshot file is unlinked after _capture_response returns,
including when the aux call raises (the finally block runs).
- Empty / malformed aux analysis falls back to the multimodal
envelope so the user always gets *something* useful.
* TestRoutingDecisionWiring (4 cases)
- Explicit auxiliary.vision in config flips routing on regardless of
main-model vision capability.
- Vision-capable main + native tool-result support keeps multimodal.
- Config load failure fails open (returns False, multimodal path
continues to work).
- Helper exception is swallowed and routes to legacy behaviour.
* TestBugReproductionAnchor (1 case) - directly pins the #24015
contract: when routing is on, the response must NEVER contain a
'data:image' or 'image_url' substring. That is exactly what tripped
the reporter's HTTP 404 ('No endpoints found that support image
input') on tencent/hy3-preview before the fix.
Bug-reproduction proof:
$ git checkout upstream/main -- tools/computer_use/tool.py
$ scripts/run_tests.sh tests/tools/test_computer_use_capture_routing.py
============================== 13 failed in 1.29s ==============================
$ # restore tool.py to this branch's HEAD
$ scripts/run_tests.sh tests/tools/test_computer_use_capture_routing.py
============================== 13 passed in 1.04s ==============================
Total branch coverage:
85 passed across test_computer_use.py, test_computer_use_vision_routing.py,
test_computer_use_capture_routing.py
|
||
|
xxxigm
|
e02a7e5e1c |
fix(computer_use): route SOM/vision captures via auxiliary.vision (#24015)
When the active main model has no vision capability — or when the user explicitly configured auxiliary.vision in config.yaml — sending the captured screenshot back to the main model in a multimodal tool-result envelope is the wrong move: it trips HTTP 404 / 400 at the provider boundary (e.g. 'No endpoints found that support image input') and the agent loop reports a hard tool failure for what should have been a simple capture. The reporter on #24015 hit this with: model: default: tencent/hy3-preview # no vision support provider: openrouter auxiliary: vision: provider: openrouter model: google/gemini-2.5-flash # explicitly configured …and observed: computer_use(action='capture', mode='som') → ⚠️ API call failed (attempt1/3): NotFoundError [HTTP 404] 🔌 Provider: openrouter Model: tencent/hy3-preview 📝 Error: HTTP 404: No endpoints found that support image input Fix: in tools/computer_use/tool.py::_capture_response, after a screenshot is captured (modes 'som' / 'vision'), consult the routing helper introduced earlier in this branch. When it says 'route to aux', materialise the PNG to $HERMES_HOME/cache/vision/, run vision_analyze on it (which honours auxiliary.vision via the standard async_call_llm task='vision' router), and return a text-only JSON tool result that embeds the analysis alongside the existing AX/SOM index. The main model never sees the pixels — it sees an actionable text description plus the same set-of-mark element index it normally uses. The two new helpers (_should_route_through_aux_vision, _route_capture_through_aux_vision) keep the policy and the IO separated so each can be tested in isolation. Both fail open: if the config import fails, if the aux call raises, or if the analysis is empty, we fall back to the existing multimodal envelope so the behaviour is at worst the pre-fix status quo. Temp screenshot files are cleaned up unconditionally in a finally block — even on aux call failure — to avoid leaving residue under cache/vision/. The end-to-end regression for #24015 is added in the next commit. |
||
|
xxxigm
|
5ce5fe3181 |
test(computer_use): cover capture vision-routing helper
Add tests/tools/test_computer_use_vision_routing.py — 28 unit tests
that pin the contract of the new vision-routing helper introduced in
the previous commit:
* TestExplicitAuxVisionOverride (12 cases): mirror the
auxiliary.vision detection rules used by agent.image_routing so
the capture path and the user-attached-image path agree on what
counts as an explicit override (provider/model/base_url with
non-blank, non-'auto' values).
* TestRouteDecision (7 cases): pin the policy itself — explicit
override always wins, vision-capable + native-tool-result keeps
multimodal, everything else fails closed and routes to aux.
* TestLookupHelpers (5 cases): defensive paths for the models.dev /
tool-result-support lookups (blank inputs, exceptions, missing
caps).
* TestModuleSurface (4 cases): pin the public/__all__ surface and
keep internal helpers addressable so the integration test in the
next commit can monkeypatch them deterministically.
Run with:
scripts/run_tests.sh tests/tools/test_computer_use_vision_routing.py
|
||
|
xxxigm
|
531efe7208 |
fix(computer_use): add helper to decide capture vision routing
Add tools/computer_use/vision_routing.py with
should_route_capture_to_aux_vision(provider, model, cfg) — a small
policy helper that decides whether a captured screenshot should be
returned as a multimodal envelope (main model has native vision) or
pre-analysed through the auxiliary.vision pipeline so the main model
only sees text.
The decision mirrors agent.image_routing.decide_image_input_mode for
user-attached images, so the capture path and the user-turn path agree
on what counts as an explicit aux vision override:
* provider/model/base_url under auxiliary.vision => explicit override
=> route through aux vision
* provider+model accepts multimodal tool results AND main model
reports supports_vision=True => keep multimodal envelope
* everything else (no tool-result image support, non-vision model,
metadata lookup failure) => fail closed and route through aux
No call sites are changed in this commit; the helper is added in
isolation so the routing decision can be unit-tested before it is
plumbed into _capture_response().
|
||
|
xxxigm
|
d35f8932e8 |
test(kanban): cover sticky blocks for worker-initiated kanban_block (#28712)
Six regression tests pinning the dispatcher contract that was broken in #28712: * test_worker_block_is_not_auto_promoted_by_recompute_ready — kanban_block survives five back-to-back ticks (compressed dispatcher loop). * test_worker_block_on_child_with_done_parents_is_still_sticky — the parent-completion code path was the worst false-positive; even when every parent is done, an explicit worker block stays blocked. * test_circuit_breaker_block_still_auto_promotes — preserves the pre-#28712 recovery semantics for circuit-breaker blocks (direct UPDATE + no "blocked" event). * test_gave_up_event_alone_does_not_make_block_sticky — explicit guard so the gave_up event is never accidentally treated as sticky; covers the second leg of the protocol_violation loop. * test_unblock_clears_sticky_state_and_lets_block_recover — only unblock_task resolves the sticky state; subsequent circuit-breaker blocks recover normally. * test_protocol_violation_loop_is_broken — full bug-shaped reproduction: block → tick → (would-be) crash + gave_up → next tick still blocked. Without the fix this would loop indefinitely. The seventh test from the original PR (legacy-DB init recovery) was dropped during salvage — the schema-init half of #28712 is already fixed on main by #28754 and #28781, and the contract is covered by test_kanban_db.py::test_connect_migrates_legacy_db_before_optional_column_indexes. |
||
|
xxxigm
|
34120a0ae2 |
fix(kanban): worker-initiated block must not be auto-promoted (#28712)
When a worker calls ``kanban_block(reason="review-required: ...")`` to hand a task off for human review, the dispatcher's ``recompute_ready`` was treating the resulting ``blocked`` status as eligible for auto-promotion — exactly the same as a circuit-breaker block. On the next tick the task flipped back to ``ready``, a fresh worker spawned, found nothing to do (work already applied, review-required comment already posted), exited cleanly, got recorded as ``protocol_violation`` → ``gave_up`` → ``blocked``, and the dispatcher promoted again. Infinite loop until manual ``hermes kanban reclaim`` + ``kanban block``. Add ``_has_sticky_block`` which distinguishes the two block sources using the cheapest available signal: the most recent ``"blocked"``/``"unblocked"`` event in ``task_events``. * Worker / operator ``kanban_block`` emits ``"blocked"`` → ``_has_sticky_block`` returns True → ``recompute_ready`` skips the task entirely. ``unblock_task`` emits ``"unblocked"`` which flips the predicate back, so the only legitimate exit is the documented human-in-the-loop path. * Circuit-breaker ``_record_task_failure`` emits ``"gave_up"`` (not ``"blocked"``) → predicate stays False → original parent-completion-recovery semantics from #40c1decb3 are preserved. * Tasks blocked purely by direct DB manipulation also recover, since they have no ``"blocked"`` event row at all — matches the existing ``test_recompute_ready_promotes_blocked_with_done_parents`` fixture behaviour. |
||
|
xxxigm
|
817e1d6340 |
test+docs(oauth): pin manual-paste semantics and document browser-only path (#26923)
Tests (``tests/hermes_cli/test_auth_manual_paste.py``): * 9 parametrised + scalar cases for ``_is_remote_session`` covering the new Cloud Shell / Codespaces / Gitpod / Replit / StackBlitz env vars (plus the existing SSH ones). * 9 cases for ``_parse_pasted_callback`` covering every paste form (full URL, https URL with extra params, bare ``?code=...``, bare ``code=...`` fragment, bare opaque value, error+description, empty, whitespace-only, malformed URL). * 3 cases for ``_prompt_manual_callback_paste`` (happy path, EOF, Ctrl-C). * 3 end-to-end ``_xai_oauth_loopback_login(manual_paste=True)`` cases: the HTTP server MUST NOT be started (asserted via a callable that raises if invoked), wrong state still rejected with ``xai_state_mismatch`` (no CSRF bypass), and empty paste surfaces ``xai_code_missing``. * SSH-hint mention test ensures the ``--manual-paste`` instruction is printed in the remote-session hint. Docs: * ``oauth-over-ssh.md`` — new "Browser-only remote (Cloud Shell / Codespaces / EC2 Instance Connect)" section with the ``--manual-paste`` recipe, plus a TL;DR note for the new flag. * ``xai-grok-oauth.md`` — short subsection pointing at the same recipe and the OAuth-over-SSH guide anchor. |
||
|
xxxigm
|
cafbc9a734 |
feat(cli): wire --manual-paste into `hermes auth add and hermes model`
Register the new ``--manual-paste`` flag on both entry points and thread it through to the xAI loopback login: * ``hermes auth add xai-oauth --manual-paste`` — pool-add path, forwarded inside ``auth_commands.handle_auth_add``. * ``hermes model --manual-paste`` — model-picker path, forwarded by ``_model_flow_xai_oauth`` into the synthetic ``argparse.Namespace`` it passes to ``_login_xai_oauth``. The picker also now forwards ``--no-browser`` and ``--timeout`` for consistency (previously hardcoded to defaults regardless of CLI flags). Help text on both flags points at #26923 and names the browser-only remote consoles (Cloud Shell, Codespaces, EC2 Instance Connect) so users searching ``hermes --help`` can find the workaround. |
||
|
xxxigm
|
5a5c265bcf |
fix(oauth): add manual-paste fallback for browser-only remote consoles
xAI Grok OAuth (and Spotify) use a loopback redirect to ``http://127.0.0.1:<port>/callback`` to capture the authorization code. That works when the browser and Hermes run on the same machine, and the SSH tunnel recipe handles the regular remote case. It breaks completely on **browser-only remote consoles** (GCP Cloud Shell, GitHub Codespaces, AWS EC2 Instance Connect, Gitpod, Replit, …) where the user has a browser but no real SSH client to forward a port — the redirect to 127.0.0.1 on the remote VM simply isn't reachable from the laptop, and there's nothing the existing flow can do about it (#26923). This commit adds the foundation for a manual-paste fallback: * ``_is_remote_session`` now also recognises Cloud Shell, Codespaces, Gitpod, Replit, StackBlitz (in addition to SSH), so the existing tunnel hint at least fires in those environments. * ``_parse_pasted_callback`` accepts any of: a full ``http(s)://...?code=...&state=...`` URL, a bare ``?code=...`` query string, a bare ``code=...&state=...`` fragment, or a bare opaque code value. Returns the same dict shape the HTTP callback handler produces, so the caller's state / error validation works unchanged (no CSRF bypass). * ``_prompt_manual_callback_paste`` reads stdin with a clear multi-line explanation of what's happening and what to paste. * ``_xai_oauth_loopback_login`` gains a ``manual_paste`` kwarg that skips the HTTP listener entirely. The redirect_uri, PKCE verifier, state, and nonce are byte-identical to the loopback path so xAI's token endpoint can't tell the difference at the protocol level. * ``_print_loopback_ssh_hint`` now also mentions ``--manual-paste`` so users without a real SSH client see a path forward instead of a dead-end tunnel recipe. * ``_login_xai_oauth`` threads ``args.manual_paste`` into the loopback helper. |
||
|
xxxigm
|
34f34ba322 |
test(xai-oauth): pin tier-denied 403 behavior + docs warning for #26847
Tests:
* ``test_refresh_xai_oauth_pure_403_marked_tier_denied_not_relogin`` —
refresh-403 raises ``xai_oauth_tier_denied`` with
``relogin_required=False`` and the API-key fallback hint in body.
* ``test_format_auth_error_tier_denied_does_not_suggest_relogin`` —
the renderer does not append "Run ``hermes model``" for the new
code.
* ``test_recover_with_credential_pool_skips_refresh_on_bare_403_for_xai_oauth`` —
bare ``{"reason":"forbidden","message":"Forbidden"}`` body (which
does not match the existing keyword heuristic) still short-circuits
``try_refresh_current`` on xai-oauth.
Docs:
* Drop the "(any active tier)" claim from the xai-grok-oauth guide,
add a top-of-page warning callout, and a Troubleshooting section
for the 403-after-login case pointing at ``XAI_API_KEY`` +
``provider: xai`` as the documented fallback.
|
||
|
xxxigm
|
3b6f57fa66 |
fix(run-agent): treat any 403 on xai-oauth as entitlement to stop refresh-loop
The existing ``_is_entitlement_failure`` heuristic only fires when
the response body contains specific substrings ("do not have an
active Grok subscription", etc.). xAI has been seen to 403 standard
SuperGrok subscribers with a terser body that doesn't match those
keywords (#26847), and the recovery path would then mint a fresh
token, get a fresh 403, and loop until Ctrl+C.
Add a defense-in-depth check at the recovery call site: any 403 on
``provider == "xai-oauth"`` short-circuits ``try_refresh_current``
so the error surfaces immediately with the friendly hint from
``_summarize_api_error``. Keeps the existing keyword path for all
other providers untouched.
|
||
|
xxxigm
|
60ef368792 |
fix(xai-oauth): split 403 (tier/entitlement) from 400/401 in token endpoint
xAI's token endpoint returns HTTP 403 to the OAuth grant when the account isn't on the allowlist for API access (e.g. standard SuperGrok subscribers — see #26847). Treating it like a stale-token 400/401 made ``format_auth_error`` append "Run ``hermes model`` to re-authenticate", which is misleading because re-login can't change xAI's tier decision. Split 403 off in both ``refresh_xai_oauth_pure`` and the loopback login token exchange: * New error code ``xai_oauth_tier_denied`` with ``relogin_required=False`` * Message explains the entitlement gate and points at the ``XAI_API_KEY`` + ``provider: xai`` fallback * 400/401 still set ``relogin_required=True`` as before * 5xx still set ``relogin_required=False`` as before |
||
|
xxxigm
|
e3f7ff1123 |
test(xai-oauth): pin PKCE token-exchange wire format
14 focused tests on the extracted helper ``_xai_oauth_exchange_code_for_tokens`` cover: Core contract: * ``code_verifier`` is on the wire (RFC 7636 §4.5). * ``code_challenge`` + ``code_challenge_method=S256`` are echoed (the #26990 defense-in-depth that makes xAI's token endpoint stop rejecting valid exchanges). * ``grant_type=authorization_code``, ``code``, ``redirect_uri``, and ``client_id`` are all locked. * Content-Type is ``application/x-www-form-urlencoded`` (xAI rejects ``application/json`` on this endpoint). * The supplied ``token_endpoint`` URL is used verbatim — no hard-coded constant sneaks in via a future refactor. * ``timeout_seconds`` is forwarded; floored at 20s. Sanity guard: * Empty ``code_verifier`` raises ``xai_pkce_verifier_missing`` with a link to #26990 — and NOTHING is sent. Leaking the auth code to a server that can't redeem it is the wrong failure mode. * Empty ``code_challenge`` omits only the defensive echo; the standards-compliant ``code_verifier`` request still goes out so RFC-compliant servers keep working. Error surfacing: * Non-200 responses include both ``HTTP <status>`` and the body verbatim — disambiguates 400 (PKCE / bad request) from 403 (tier denied, see #26847). * Transport errors are wrapped as ``AuthError`` with the ``xai_token_exchange_failed`` code, so the surrounding ``format_auth_error`` UI mapping still fires. * Non-dict JSON payloads raise ``xai_token_exchange_invalid``. * 200 happy path returns the parsed payload dict verbatim. End-to-end wire-format guard: * A real ``httpx.Client`` with a stub transport captures the bytes on the wire and asserts every PKCE field round-trips through ``urlencode``. Catches a future refactor that swaps ``data=`` for ``json=`` (which xAI would silently reject). |
||
|
xxxigm
|
cb53c40e45 |
fix(xai-oauth): echo code_challenge in token POST so PKCE exchange succeeds
xAI's OAuth implementation at ``auth.x.ai`` validates the PKCE ``code_challenge`` at the **token** endpoint, not just at the authorize step. When Hermes sends the standards-compliant token POST with ``code_verifier`` alone — exactly what RFC 7636 §4.5 prescribes — xAI rejects the exchange with ``code_challenge is required`` and the user is stuck with no working OAuth login. The fix: * Extract the token POST into ``_xai_oauth_exchange_code_for_tokens`` so the wire format is unit-testable in isolation. * Send the original ``code_challenge`` and ``code_challenge_method`` in the form body alongside ``code_verifier``. Strict RFC-compliant servers ignore the extras at the token endpoint, and xAI's permissive implementation accepts the exchange. This is the standard "defensive echo" workaround used by every OAuth client that targets a server with this quirk. * Refuse to fire the POST when ``code_verifier`` is empty — leaking the authorization code to a server that can't redeem it is worse than failing locally with an actionable error. The new error code is ``xai_pkce_verifier_missing`` and the message points at this issue for context. * Surface the HTTP status code prominently in the 4xx error message (``xAI token exchange failed (HTTP 400). Response: …``) so users and maintainers can tell a 400 (bad request / PKCE problem) from a 403 (tier denied, see #26847) at a glance instead of parsing the JSON body by eye. Closes #26990 |
||
|
xxxigm
|
cdc0a47dd5 | test(hermes_constants): cover parse_reasoning_effort() | ||
|
xxxigm
|
d5fcc83922 | fix(tests): avoid asyncio DeprecationWarning in event loop fixture on 3.12+ | ||
|
xxxigmandCursor
|
647f95b422 |
docs(contributing): align tool discovery and test runner with AGENTS.md
Co-authored-by: Cursor <cursoragent@cursor.com> |