_credentials coerced every value with str(), so YAML channel_secret: true
keyed the HMAC with the guessable "True" (likewise 123, lists, maps).
Read a value only when it is a string, matching the webhook adapter's
_is_usable_secret policy; scoped-env values stay first. (Lead fold: Sol
r4/r5 + quality lens, converged; probed: true/0 -> "" on head.)
HMAC zero-pads its key, so a whsec_ secret that decodes to NUL bytes
(whsec_AA==) signs exactly like the empty key b"" the previous fold
refuses; the whitespace-only guard let it through. Strip NULs with the
ASCII whitespace before the emptiness check. (Lead fold, 2c lens Low.)
_is_usable_secret only sees the encoded string, so "whsec_" (decodes to b"")
and "whsec_ICAg" (decodes to three spaces) passed it and keyed the Svix HMAC
with a public value: any caller could sign a request. Apply the same
blank-key rule to the decoded key before computing the HMAC.
`bridge_token:` with an empty YAML value became str(None) == "None" (and a
whitespace-only value stayed " "). Both are truthy, so connect() skipped
the auto-generated token and _authorized() accepted any wake/activity POST
carrying that header. Same class as the null-secret fixes for LINE and
WhatsApp Cloud in this stack.
Reading the standalone-send token through _credentials (earlier in this
stack) strips it, but no test failed when that read was reverted: the only
standalone test used an absent token. A whitespace-only token now has to
stop at "missing token" before the push API.
PlatformConfig keeps a numeric YAML api_key as an int, so the .strip() this
stack added to _standalone_send raised AttributeError where origin/main
returned an error dict. Coerce through str(... or "") like the LINE reader,
and pin both the int key and a whitespace-only key (which also gives the
standalone strip a test that fails without it).
An empty YAML value (`app_secret:` / `verify_token:`) arrives as None, and
str(None) made the inbound signing key the literal "None". The truthiness
guard in _handle_webhook then accepted it, so anyone could sign a POST with
HMAC("None", body). Same class as the LINE null-secret fix in this stack;
`or ""` lets a null read as unset and reach the existing 503 refusal.
_standalone_send still read the access token raw, so a blank or newline-padded token went out as the Bearer header; reuse the hardened _credentials. Pin the token half of the null/blank guard and the env presence strip (both reverted green before). Gate r4 Lows.
Reverting only the check_sms_requirements strip left the suite green; also drop the dead 'or ""' after reads that already pass a default. Gate r3 Lows.
c92ed7e000 wrapped the extra fallback in str(), so a present-but-null config value (an empty YAML key) became the guessable literal "None" and connect passed. Fall through to "" before str(); strip the env presence check too so check_requirements agrees with connect. Gate r3 High (2ab, 2c, quality converge).
Gate r2 findings 3 and 4. Mutating _dynamic_route_allowed to
`if not effective_secret:` stayed green, because no test reached the
hot-reload guard with a blank or non-string secret. The existing
parametrized connect case now asserts the dynamic-route guard rejects
the same secrets, and it goes red under that mutation (4/4). Also
rename the test and its param to match what they cover (unusable, not
only whitespace-only), and refresh the stale docstring to name
_is_usable_secret.
Gate r2 finding 2 (same class as the webhook blank-secret fix):
TWILIO_AUTH_TOKEN was read unstripped, so a whitespace-only token
passed check_sms_requirements and keyed the X-Twilio-Signature HMAC
with blanks, letting anyone forge inbound SMS. Strip the token at
every read (adapter init, standalone send, requirements check).
Stripping alone would leave an empty key, and HMAC with an empty key
is equally public (verified by probe: a signature forged with b''
was accepted). So _check_signature now refuses outright when no
auth token is set. This goes one line past the reviewer's FIX, which
only asked for the strip.
Gate r2 finding 1 (same class as the webhook blank-secret fix):
_credentials returned LINE_CHANNEL_SECRET / extra['channel_secret']
unstripped, so a whitespace-only secret passed the `not channel_secret`
check at connect and in verify_line_signature, and the webhook HMAC
was keyed with blanks, making signatures forgeable. Strip both values
in _credentials (matching whatsapp_cloud) so a blank secret reads as
unset and connect refuses with config_missing.
Fold finding 2 (gate): _is_usable_secret now also rejects non-string
secrets (a YAML int like `secret: 123` fails closed), but the startup
ValueError, the dynamic-route warning, the per-request log and the
comment in _read_authenticated_body still said "no secret" / "missing
or empty". An operator with an int secret would be told it is absent.
All three now say "missing, blank, or not a string"; the connect()
test gains an int row and matches the new wording.
Fold finding 1 (gate, 3 reviewers): is_usable_secret lived in the
experimental gateway/relay/auth.py, so every webhook import pulled in
the relay package (~2.5ms) for a one-line predicate only the webhook
adapter uses. Move it next to _hex_hmac as module-private
_is_usable_secret, revert both relay/auth.py hunks, and move the
parametrized classifier test into tests/gateway/test_webhook_adapter.py.
No behaviour change.
The salvaged tests only covered connect(); extend the existing
missing-secret request test with a whitespace-only route secret signed
with that same secret, which BASE authenticated. Drop the
real-secret-accepted connect case, already covered elsewhere, to keep the
stack at two invariant tests.
A blank or whitespace-only secret now fails closed at webhook startup,
on the request path, and when a dynamic route is admitted.
(cherry picked from commit 05e918d52c41e905d88e6c0a0f55e7612f4d899d)
The code-health ratchet (BLE001) rejects a blind except Exception that
neither re-raises nor logs the traceback. The record path already logs
at debug with exc_info; give the lookup path the same.
Cloud spectrum targets carry no direction and _sent_message_ids is
in-memory, so after a gateway restart a swipe-reply to one of our
bubbles hydrated the quoted text from the sent-text index but was
marked as not ours: the agent saw [Replying to: "..."] instead of
[Replying to your previous message: "..."]. Only our sends are indexed,
so an index hit is itself proof the target was ours.
Polls (clarify), the plain-text fallback resend and standalone attachment
sends reached the sidecar without recording what they sent, so a threaded
reply to those bubbles lost its quoted context. Record once in _post_send
(plus the standalone attachment leg, which has its own HTTP client) and
reuse _normalize_chat_key instead of a duplicate regex.
spectrum-ts 12.x delivers a threaded (swipe) reply as
{type: 'reply', content, target}. The sidecar had no branch for it, so
the agent received '[Photon content type not handled: reply]' and the
message text was lost.
- sidecar: normalize 'reply' to its inner content plus the target's id,
direction and text
- adapter: unwrap it and set reply_to_message_id / reply_to_text /
reply_to_is_own_message on the MessageEvent
- adapter: record outbound text (send, attachments, standalone send) in
gateway.rich_sent_store and use it when spectrum can't hydrate the
quoted text, which is the usual case for our own bubbles. DM GUIDs
and bare E.164 share one key.
OpenViking now ships from Volcengine's own repo through the plugin catalog
(plugin-catalog/openviking.yaml, pinned in #131267). Delete the in-tree copy
and its tests; memory_provider_migration already lists openviking in
_LEFT_CORE, so homes configured for it get the catalog plugin installed on
hermes update / first agent start.
The catalog pin starts openviking-server with the full Hermes environment
(the bundled copy filtered it). The docs now say so and point at
volcengine/OpenViking#5553; start openviking-server yourself to avoid it.
Review fold (round 1, simplify reuse + quality):
- tokenhub_effort() in agent/reasoning_effort.py: the tencent-tokenhub profile and the host-based
branch base_url-only agents take used the same "high unless chosen, else clamp" expression in
two places, so the two paths could drift apart.
- generic_nested_reasoning(): the OpenAI-compatible extra_body fallback auxiliary calls send when a
profile declares no reasoning shape. The lmstudio profile's auxiliary branch was a line-for-line
copy (it must produce it itself because overriding build_api_kwargs_extras turns the aux
fallback off). Both now call the helper.
- agent/lmstudio_reasoning.py docstring names its real caller (the lmstudio profile).
Behaviour unchanged: 42/42 main-loop request cells and every auxiliary projection cell are
byte-identical to origin/main.
- Plugin hooks written against the older build_api_kwargs_extras keywords (no **context) no
longer raise TypeError on every request: lmstudio_reasoning_options is forwarded only when the
agent probed it (LM Studio). Pinned by TestProfileHookKeywordCompat, which fails if the field is
passed unconditionally.
- tencent-tokenhub: drop base_url and supports_model_listing=False. With them, first-time
`hermes model` setup reached the profile path and offered only the 3 curated Hy models instead
of probing {base}/models as main does; without a profile base_url, setup and the /model picker
take the same paths they took with no profile.
- One name for LM Studio's probed options end to end (lmstudio_reasoning_options), matching the
other provider-prefixed hook keys (ollama_num_ctx, qwen_session_metadata).
- New base-url:github-models-chat snapshot cell (openai/o4-mini, asserted chat_completions): the
gpt-5.5 cell goes to Responses and never reached the legacy GitHub reasoning branch.
- Comment/docstring truth: the host branches serve agents with no registered profile (not only
base_url-only ones); the LM Studio effort omission is done by the profile.
lmstudio and tencent-tokenhub were the last chat-completions providers with no ProviderProfile,
so their requests were built by the legacy flag branch in ChatCompletionsTransport.build_kwargs
(name check for LM Studio, host check for TokenHub). They now have profiles that reproduce the
same wire through build_api_kwargs_extras:
- lmstudio: top-level reasoning_effort from the model's published allowed_options. The agent's
cached probe is passed to the hook as reasoning_options; auxiliary calls (no probe) keep the
generic nested reasoning they already sent. base_url is left empty on purpose: a loopback
base_url would map every 127.0.0.1 / localhost endpoint to LM Studio in URL-to-provider
inference and turn off local-server detection for Ollama, llama.cpp and vLLM. Model listing
and the doctor probe stay off so the picker keeps LM Studio's own chat-model probe.
- tencent-tokenhub: top-level reasoning_effort clamped to low/medium/high, omitted when thinking
is off, high when no level is chosen (default_reasoning_config on main-loop turns). Model
listing and the doctor probe stay off so the picker keeps the curated Hy list.
The name-based LM Studio branch is removed from the legacy path. The host-based branches stay:
an agent built from a base_url alone (run_agent.py main, the batch runner, library callers)
carries provider="" and still reaches the legacy path. 11 base_url-only snapshot cells now pin
those host-detected requests (OpenRouter, Nous, Kimi, Kimi coding, TokenHub, GitHub Models,
NVIDIA NIM, Qwen portal, a localhost LM Studio port, Ollama, a generic endpoint) so the change
that retires the legacy path can prove it sends the same requests.
Deliberate deltas: model ids prefixed lmstudio: / tencent-tokenhub: are now stripped like every
other profiled provider's; TokenHub auxiliary calls with a reasoning config send top-level
reasoning_effort (the main-loop shape) instead of nested extra_body.reasoning.
Review threads left open on three merged PRs, each a gap in what the tests actually guard:
- #133740 (relay usage merge): no checked-in test failed on the base. The merged fix keeps
message_start's input/cache counts when the SDK's MessageDeltaUsage serializes them as null,
but the existing message_delta test only sent output_tokens, so it passed with the old merge
too. New test_anthropic_stream_accumulator_null_delta_usage_keeps_message_start_counts sends
nulls for input and cache fields; it fails with the old `{**current_usage, **usage}` merge.
- #133767 (error-body table): the #49769 cell asserted the 402 would be reclassified as
retryable. The fix that landed (#134157) keeps it `billing` and retries in the main loop's
recovery chain, so the cell still XFAILed on main and would pass even if the retry broke.
It now pins what the recovery depends on (billing + a parseable budget); the retry itself is
driven end to end by test_affordable_402_main_loop.py.
- #133767 (prompt-cache contract): the marker check needed cache_control in only one request,
so a later request that lost its markers passed. It now requires markers on every request.
The module docstring claimed native Anthropic layout coverage the cells don't have; it now
says envelope layout.
Greptile on a6b51f6ed4e (5/5, non-blocking): `_detect_upstream_elision`
only scans strings, so once an MCP result also carries an image it reaches
`make_tool_result_message` as a part list and an elided payload
("... 13 more items", has_more:true) lost the "data shown is INCOMPLETE"
notice. A part list is now judged by its first text part, and the notice
is appended there, inside the untrusted wrapper; the image is untouched.
Greptile on b17398db089:
- `enforce_turn_budget` took `len(content)` of every tool message. An
image-bearing result reaches it as a part list, so it counted parts,
not characters: six 60K-char MCP results with screenshots totalled
~18 "chars" and nothing was spilled. Part lists now count their text
characters and spill their largest text part (images stay inline).
- `_persist_multimodal_text_parts` spilled every text part under the same
tool_call_id, so with a small `tool_budget.mcp_result_size_chars` the
notes part overwrote the tool text's spill file. Each spilled part now
gets its own id.
- When `vision.max_calls_per_image` refused every image, the refusal lines
were appended after the handler's JSON closing brace. They now go into
the envelope's `result`.
Greptile on 1c7dfff98e7: the scale notes sat at the end of the
envelope's single text part. When the tool text is over the result-size
limit, `_persist_multimodal_text_parts` spills that whole part to a file
and keeps only its head, so the resized screenshot stayed attached
without the "multiply coordinates by N" map and clicks would land on the
wrong pixel. The header and notes are now their own short text part,
which stays inline.
Greptile on 0ce63424a06:
- Only the first four images of a result were prepared; a damaged or
already-in-context image among them hid a valid fifth. Images now
prepare in waves sized to the free slots (bounded at 16 candidates), so
a skip hands its slot to the next image.
- The attach check compared against the 20 MB hard cap, not
`vision.embed_target_bytes`; a best-effort resize that bottomed out
above a low budget rode history anyway. It now checks the configured
target (clamped to the hard cap).
- `tools/computer_use/vision_routing.py` had become a one-line negation
of `_native_tool_result_images`. The capture boundary calls the shared
gate directly; the module and its tautological agreement test are gone,
and the route-decision cases move into the capture-routing tests where
they drive `_should_route_through_aux_vision` itself.
Greptile on 6c01b824fb: moving the prep onto `_vision_cpu_executor`
dropped the calling session's ContextVars. The active runtime is
context-local (`set_runtime_main`), so a pool thread fell back to the
process default and `_supported_media_types` no longer saw a managed
llama.cpp runtime: a small WebP was attached unchanged, and stb_image
cannot decode it (the model confabulates). Profile-scoped vision settings
had the same gap. Each job now runs in `contextvars.copy_context().run`,
as the delegation and MCP-loop paths already do.
Greptile re-review of 6771a0e681:
- A polled MCP screenshot tool re-caches identical pixels under a new
path on every call, so each copy was embedded and re-sent on every later
request, past the per-image cap (3 in delegated children by default).
Each attachment now reserves through `repeat_refusal`, keyed on the
resized data URL (same pixels, same key); a refused image keeps its
MEDIA: path with an "already in context" line.
- Decode/resize ran on the tool thread, so a parallel tool batch of
image-heavy MCP calls could decode many large images at once on a shared
gateway. Prep now runs on the bounded `_vision_cpu_executor`
(auxiliary.vision.max_concurrency), as vision_analyze's encodes do.
Independent review follow-up:
- The shared resizer is best-effort: its 64 px short-edge floor keeps a
60000x64 image at 60000 px, and the MCP path checked only bytes, so an
over-limit part could enter history (providers reject >8000 px per
turn and the reactive shrink cannot fix it). The attach path now
refuses any image whose output long edge is still above 1568 px; the
result stays the MEDIA: text.
- A resized screenshot lost its coordinate mapping. The MCP path now
passes scale_out and lists `_build_scale_note` per MEDIA: path, as
vision_analyze does, so a cua-driver-over-MCP click lands on the screen
pixel the model saw.
- computer_use cached its route per (home, provider, model) for the
process, so editing `image_input_mode` (or the aux backend) never
reached the next capture. The cache is gone: the decision reads the
signature-cached `load_config_readonly` per capture. Pre-existing on
main (12ad13ddca), fixed here because this PR claims every image tool
honours the setting; the perf test that pinned the stale cache becomes
the "config change applies without restart" invariant.
Review follow-up on the MCP native-image path (Greptile):
- A JPEG/WebP/GIF with a valid header over a truncated pixel stream passes
both the cache's magic-byte check and the sniff, and one undecodable part
makes the provider reject the whole request. The cached file is now
fully decoded (`_validate_raster_image_decodable`, as vision_analyze)
before it is attached; on failure the result stays the MEDIA: text.
- tools/AGENTS.md names the multimodal envelope as the one sanctioned
non-string handler return, routed through `_native_tool_result_images`.
- The invariant tests register the fake server with
`_register_server_tools` and call `registry.dispatch`, so dropping
`native_images=True` at registration goes red. A truncated JPEG case
pins the decode check (red when the check is removed).
The capture route only checked "does the provider carry tool-result
media", never `agent.image_input_mode` or a catalog verdict that the
model cannot see. deepseek-v3.2 on OpenRouter therefore went native, the
agent backstop swapped the screenshot for a "switch to a vision-capable
model" error, and the configured auxiliary vision describer never ran;
`image_input_mode: text` was ignored for screenshots too.
`tools.vision_tools._native_tool_result_images(provider, model, cfg)` is
now the one gate for every tool that hands the model pixels
(vision_analyze, browser screenshots, computer_use captures, MCP images):
image routing resolves to native AND the route accepts tool-result
images. The capture route is its negation, which drops three local
helpers that duplicated image_routing. Proxy aliases the catalog does
not know still go native under `image_input_mode: native` (#115248's
case); `auto` sends them to aux, matching vision_analyze.
Tests: test_computer_use_vision_routing.py is rewritten to drive the
real gate (the old cases patched the removed helpers). Changed
assertions: "whitelisted provider + catalog-unknown model" now requires
image_input_mode: native (it carried that cfg already); new cases pin
catalog text-only -> aux and image_input_mode: text -> aux.
An MCP tool's ImageContent was cached and the model got back only a
`MEDIA:/path` line, so a vision main model could never read the image
(a raw cua-driver MCP server could not see the screen at all).
The registry handler now returns the `_multimodal` envelope when the
active route takes images inside tool results, decided by the same gate
vision_analyze and browser screenshots use (`agent.image_input_mode`, an
explicit `auxiliary.vision` backend, catalog vision, provider support).
The image is re-encoded from the sniffed cache file, normalized to a
provider-accepted format and sized to `vision.embed_target_bytes` /
1568 px (it is re-sent every later turn), at most 4 per result; the text
half keeps the MEDIA: paths. Paths come from this call's own cache
writes, never parsed out of server text. The resize runs on the tool
thread, off the shared MCP loop. Plugins' `ctx.call_mcp` keeps its
string contract (native_images is set only on the registry handler).
Builds on #131374 by @sarahwu185, which reused the computer_use capture
gate (ignores image_input_mode, native for catalog text-only models) and
inlined the server's raw base64 unvalidated and unresized.
From #29082: board_stats() counts every non-archived status, so the plain-text
`By status:` block must list `review` too, and agree with `stats --json`.
Adapted to the current test module (task enters review via request_review).
The code-health ratchet caps hermes_cli/kanban_db.py at its main size (4585);
the module import + derived assignment added two lines. An aliased import of
DEFAULT_STATUSES replaces the literal set one-for-one, so the facade stays at
4585 and every kb.VALID_STATUSES reader is unchanged.
The manual table now has an `archived` row (restore), so the "archive is not
listed" comment was half-wrong and GET /workflow sends a manual key that is
not a column. Say so where the shape is defined.
- can_move: `archived` is a valid source (restore). The dashboard PATCH accepts
archived -> triage/todo/ready (measured live), so the manual table gains that
row and the matrix test now measures it too. A same-column drop returns False
instead of raising or meaning "allowed".
- `hermes kanban stats` text output iterated its own hardcoded status list,
which omitted `review`; it now loops over DEFAULT_WORKFLOW.keys().
- Tests: drop three asserts that could not fail (VALID_STATUSES is
DEFAULT_STATUSES; BOARD_COLUMNS is built from keys()); add one invariant:
can_move answers for every value tasks.status can hold.
Review of #130067: traits described kernel meaning nothing reads (and two
descriptions were incomplete: archived also satisfies a parent gate; the
kernel re-checks parents on ready/review). Same reasoning as the dropped
event model: Phase 0 keeps only data that is consumed or checked against
live code; column semantics are designed with the kernel switch.
Nits: VALID_STATUSES is the frozenset itself; precise tuple/frozenset types.
Review of #130067: the event edges claimed every block lands in 'blocked',
but _route_block routes to todo / triage / blocked, and ~20 kernel status
writes are conditional (crash/reclaim return, failure breaker, parent
reopen, import). Rather than publish a partial contract, Phase 0 keeps only
what is checked now (columns, traits, manual matrix, derived copies); the
event model is designed with the kernel switch in Phase 1.
Also from review:
- can_move rejects unknown column keys instead of defaulting (archive of a
bogus key returned True).
- the matrix test only accepts 400/409 as a refusal; 5xx or 422 fails.
Groundwork for user-defined columns and workflow per board (#54818).
- hermes_cli/kanban_workflow.py: Column/Workflow + DEFAULT_WORKFLOW, today's
board written down as data (column order, traits, default kernel-event
edges, manual move allow-list). Not consulted by the kernel yet.
- VALID_STATUSES, dashboard BOARD_COLUMNS, the kanban_list status enum and
CLI status icons now derive from it. Fixes drift: the tool enum lacked
scheduled/review; CLI icons lacked triage/review.
- GET /workflow on the dashboard plugin API.
- Tests pin DEFAULT_WORKFLOW to real behavior: every default event edge vs.
its kernel verb, and all 64 PATCH src->dst moves vs. can_move.
