fix(state): read-only session search silently returned no results; add stateless memory/session_search MCP shims - #65978
Conversation
1eaa864 to
5a3d7cf
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Looks good. No obvious issues found.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Prior COMMENT review noted. Medium PR (644 additions) — read-only session search fix + stateless memory/session_search MCP shims. Addresses silent failure when session search returns no results.
Key observations:
- Adds shims for memory/session_search MCP in read-only contexts
- Fixes silent failure path for stateless scenarios
- No security concerns
- No debug artifacts
Clean fix for the described issue. LGTM.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
Fix: model wizard now recognizes credential pool keys in auth.json. Allows credentials added via auth add to appear in model selection.
Clean fix. No security concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real read-only FTS defect and for preserving the generic _AGENT_LOOP_TOOLS refusal. Current main confirms the premise: the read-only SessionDB branch returns without setting _fts_enabled (hermes_state.py:1030-1047), and search_messages() then returns [] (hermes_state.py:5277-5278). The stateless MCP direction also matches the open maintainer-owned request in #26604.
Problems
- The session-search shim's own-lineage exclusion is not wired to the Codex MCP lifecycle. It reads
HERMES_MCP_SESSION_IDatagent/transports/hermes_tools_mcp_server.py:286, but the managed MCP entry only suppliesHERMES_HOME,PYTHONPATH, quiet mode, and redaction (hermes_cli/codex_runtime_plugin_migration.py:568-601). A repository-wide search found no producer of that session-id variable, so ordinary calls usecurrent_session_id=Noneand can return the active session.
Suggested changes
- Carry the active Hermes session id through a session-aware MCP launch/request path and add an end-to-end regression covering exclusion of the current lineage. If that transport is not feasible for the shared stdio server, narrow the shim contract rather than claiming native exclusion.
Automated hermes-sweeper review.
| sort=kwargs.get("sort"), | ||
| profile=kwargs.get("profile"), | ||
| db=db, | ||
| current_session_id=os.environ.get(_SESSION_ID_ENV, "").strip() or None, |
There was a problem hiding this comment.
HERMES_MCP_SESSION_ID has no producer in the managed Codex MCP launch path: hermes_cli/codex_runtime_plugin_migration.py:568-601 only sets HERMES_HOME/PYTHONPATH/quiet/redaction. This is therefore None in normal runtime use and current-session lineage is not excluded. Please wire session-aware propagation and cover the real launch path, or narrow this behavior claim.
There was a problem hiding this comment.
You're right, and thanks for the precise pointer — _build_hermes_tools_mcp_entry() sets only HERMES_HOME / PYTHONPATH / HERMES_QUIET / HERMES_REDACT_SECRETS. I checked repo-wide across all file types: HERMES_MCP_SESSION_ID had no producer anywhere in product code — only the consumer, two doc comments, and two monkeypatch.setenv calls in tests. The one test that looked like coverage set the variable itself, so it proved the shim's internal plumbing and nothing about the launch path. In normal runtime it was None and own-lineage exclusion never ran.
I also don't think it should be wired where you pointed: that entry is serialized into ~/.codex/config.toml at migrate time, so a per-session id written there would be burned in stale — the same failure mode the _looks_like_test_tempdir guard already exists to prevent. There's now a test forbidding it.
The real mistake was inventing a variable that duplicates one Hermes already has. HERMES_SESSION_ID is written by set_current_session_id() (gateway/session_context.py:153), is in _VAR_MAP, and is bridged into the codex spawn env by _inject_session_context_env() inside hermes_subprocess_env() — the same inheritance HERMES_KANBAN_TASK relies on (codex_app_server.py:102). The value was already arriving; the shim was reading the wrong name.
Fixed by deleting HERMES_MCP_SESSION_ID and reading the canonical HERMES_SESSION_ID. No launch-path change, and deliberately no new HERMES_* variable — I didn't keep the old name as an override, since per AGENTS.md that would be a new non-secret env var with no consumer. Reading the canonical var also picks up the cross-session leak guard in _inject_session_context_env for free, which turns out to matter: a hand-rolled producer for the bespoke name would have sat outside that guard and could have carried a sibling session's id under a concurrent multi-session host, excluding the wrong lineage — a silent wrong answer rather than a silent no-op.
Tests: the fixture now clears HERMES_SESSION_ID (otherwise a developer's live session id leaks into the suite and the new tests pass spuriously), and exclusion is driven through the real producer via set_current_session_id() instead of setenv, so a producer/consumer name mismatch fails loudly instead of hiding. Both new tests were confirmed red before the fix.
While in there I audited the rest of the shim docs and corrected four claims that outran the code:
- the stale "we deliberately do not expose memory / session_search" note — this PR exposes them — plus its wrong line reference (
model_tools.py:493; it's:606, gate at:1167); - "faithful stateless equivalents" — not true for
session_search, which adds a deterministic zero-hit OR-relaxation the native tool doesn't have; - the
claude-agent-sdkreference — that provider isn't in this PR; - the consolidation-failure breaker listed as "inherited" — a fresh store per call resets its counter, so it can never trip here.
Also documented the staged-write caveat (with no foreground approver in a stdio subprocess the approval gate can return staged, so a write reports success without landing yet) and the fail-open behaviour when no session id is present — exclusion is simply inactive, no error.
One thing I'd rather state than overclaim: I verified that codex's spawn env carries HERMES_SESSION_ID by reading the code path and the HERMES_KANBAN_TASK precedent that production gating already depends on. I have not instrumented the codex binary itself to confirm it forwards inherited env to the hermes-tools child.
Not addressed here, flagging as a follow-up: CodexAppServerSession doesn't accept or forward env= to its client factory, so CodexAppServerClient(env=...) is effectively dead on that path.
There was a problem hiding this comment.
One correction to my reply above, before anyone relies on it: "the value was already arriving" holds only where the MCP host forwards the parent env — and the codex-managed launch specifically does not. Codex builds each MCP subprocess env from scratch (create_env_for_mcp_server, codex-rs/rmcp-client/src/utils.rs): a fixed whitelist (HOME/PATH/…), plus names listed in the entry's env_vars, plus the entry's literal env map — the parent env is not inherited. Our migration entry sets only HERMES_HOME/PYTHONPATH/HERMES_QUIET/HERMES_REDACT_SECRETS, so HERMES_SESSION_ID reaches the codex process (via _inject_session_context_env) and is then dropped at the codex→MCP hop. On that path own-lineage exclusion remains inactive (fail-open) — i.e., the "narrow this behavior claim" branch of your review applies to it, and the shim docs' fail-open wording is the accurate description there.
The clean wiring does exist on codex main: env_vars in the MCP server entry is a name-passthrough resolved from the codex process env at spawn time, so listing "HERMES_SESSION_ID" propagates the live id without serializing a value into config.toml (the burned-in-stale problem that ruled out the literal-env route). I can add that to _build_hermes_tools_mcp_entry with a launch-path regression here, or leave the contract narrowed as documented — happy to take direction. (For completeness: the stacked #65982 runtime sets the id explicitly in its MCP env map, so exclusion is active on that path.)
There was a problem hiding this comment.
Closing the loop on this thread with current state (2026-08-13): the producer gap is closed in code — b3b3508c0f wires the active session id through the codex MCP lifecycle by naming HERMES_SESSION_ID in the MCP entry's env_vars (codex's spawn-time name-passthrough delivers it), and d920b47ab adds the launch-path regression covering exactly the fail-open scenario you flagged. Also re-verified today: the branch merges clean against current main (bd6dcd4bd5-era) with all checks green, and the added claude_sdk_session_id column is safely picked up on pre-existing DBs by the declarative reconciler (_reconcile_columns) — empirically tested against a v25 DB built without the column. From our side the thread is addressed; happy to adjust if you see a remaining gap.
3aaf1b1 to
9756d26
Compare
|
@teknium1 — bumping the one open thread here, a week on (2026-07-19): own-lineage exclusion is fail-open on the codex-managed MCP path because codex builds each MCP subprocess env from scratch and the migration entry doesn't pass |
|
The keep_open either/or is resolved in code as option (a) — the active session id wired through the codex MCP lifecycle, end-to-end regression included:
Gate: the full suite's failure set is identical to the branch's pre-change baseline (same pre-existing machine-env failures, none added, none removed). With (a) landed the contract is no longer narrowed, so the (b) acceptance question dissolves — but if maintainers still prefer the narrowed-contract documentation for other hosts, happy to add it on top. |
5d8cc62 to
80fdedc
Compare
…k_session_id column on sessions Read-only handles skipped the FTS probe, so search_messages() silently returned [] on every ro connection — a false empty, not a degrade. Only a MISSING fts object disables search; a transient error (e.g. a lock during a checkpoint) leaves it enabled so the query surfaces the failure visibly. The new nullable column (declarative migration via SCHEMA_SQL reconciliation) lets a runtime persist its provider-side session id for cross-restart resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s-tools MCP server The two agent-loop tools have faithful stateless equivalents: memory via a fresh load_on_disk_store() per call (native caps, drift guard, threat scan, locking inherited) and session_search via a read-only SessionDB, with the calling session's id riding HERMES_MCP_SESSION_ID. The _AGENT_LOOP_TOOLS refusal in handle_function_call stays intact for every other caller. A zero-hit multi-term discovery query is retried once with OR-joined terms (FTS5 ANDs terms; annotated, never silent, never overriding explicit operators). A present-but-uninitialized state DB degrades to an explicit error, never a silent empty result. Because a stateless shim write cannot mirror through MemoryProvider hooks, a configured external memory.provider backend unregisters the memory shim and refuses at dispatch — fail-closed over silent store divergence. Implements the memory/session_search shim tracked in NousResearch#26604 (origin: NousResearch#26567). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…producer HERMES_MCP_SESSION_ID had no producer in any launch path — only the consumer, two doc comments and two monkeypatch.setenv calls in tests — so own-lineage exclusion never ran outside the suite. The canonical HERMES_SESSION_ID is already written by set_current_session_id(), carried in _VAR_MAP and bridged into the codex spawn env by _inject_session_context_env() inside hermes_subprocess_env() — the same channel HERMES_KANBAN_TASK already rides. Read that instead of a bespoke name; no launch-path change is needed. Hand-rolling a producer for the bespoke name would have been worse than the bug: the cross-session leak guard in _inject_session_context_env covers only _VAR_MAP keys, so under a concurrent multi-session host it could carry a sibling session's id and exclude the WRONG lineage — a silent wrong answer instead of a silent no-op. A session id must also never enter the codex config.toml entry, which is serialized at migrate time and would burn in stale; that is now forbidden by a test. The new tests establish the precondition through the real producer (set_current_session_id) rather than setenv, so a producer/consumer name mismatch fails loudly, and the fixture clears HERMES_SESSION_ID so a developer's live session cannot make them pass spuriously. Also corrected four claims in the shim docs that outran the code: the stale "we deliberately do not expose memory / session_search" note plus its wrong model_tools.py line reference (:493 -> :606), "faithful stateless equivalents" (session_search adds a deterministic OR-relaxation the native tool does not have), the claude-agent-sdk reference (that provider is not in this PR), and the consolidation-failure breaker (a fresh store per call resets its counter, so it can never trip here). Documented the staged-write caveat and the fail-open behaviour when no session id is present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… env whitelist drops The three sites (server constant, migration-test docstring, shims-test comments) described HERMES_SESSION_ID as delivered to the shim by _inject_session_context_env() — that chain ends at the HOST process env (hop 1). codex builds an MCP child's env from a fixed whitelist plus the names listed in the entry's env_vars (a spawn-time snapshot); the migration entry names none, so under codex the shim never sees the var and own-lineage exclusion is INACTIVE (fail-open), exactly as the server doc block already states. Reworded to say where delivery actually happens, dropped the HERMES_KANBAN_TASK analogy (that var rides plain env inheritance, not the _VAR_MAP bridge, and is equally dropped at the codex hop), and narrowed the 'End-to-end / fails LOUDLY' docstring to the name contract the in-process test actually pins. The burn-in rule (never a literal session id in config.toml) and the sibling-leak rationale for the canonical name are unchanged — both were correct. Follows up review thread 3609530775; whether to wire env_vars into the migration entry stays the maintainer's call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s probe beside it
The read-only trigram probe latched _trigram_available=False on ANY
sqlite3.Error — two lines below the messages_fts probe that classifies
via _fts_object_missing, and directly under the block comment stating
the rule ('a transient error must not latch ... for the handle's
lifetime'). A 'database is locked' during a checkpoint pinned every CJK
query on the LIKE fallback (which ORs tokens, drops NOT, and ignores
rank) for the handle's life.
Absence has one extra spelling the shared classifier does not cover: a
build with FTS5 but without the trigram tokenizer raises 'no such
tokenizer: trigram', so the probe consults _is_trigram_unavailable_error
too — without that, such a build would latch True forever and silently
re-fail every CJK query. Keeping True on a transient is safe by
construction: search_messages catches the per-query OperationalError and
falls through to LIKE; that handler now also logs once (via the existing
_warn_trigram_unavailable) when the failure is the permanent
missing-tokenizer case.
Tests were planted red first: the transient case fails on the previous
latch, and the missing-tokenizer case fails on the naive
'not _fts_object_missing(exc)' variant.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…h are unavailable — the shims made that false The page claimed four times that memory and session_search need the agent loop and cannot work on the codex runtime. Since the stateless shims in hermes_tools_mcp_server, both ARE served through the MCP callback. Describe the shim reality honestly instead: session_search cannot exclude the current conversation's lineage under codex (the session id never crosses codex's env whitelist — fail-open, documented), and the memory shim fails closed when an external memory provider is configured because a shim write cannot mirror through MemoryProvider hooks. delegate_task and todo remain genuinely unavailable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…probe
The transient-vs-absent tests only intercepted the trigram probe SQL, so
the primary probe's classification ('database is locked' must keep
_fts_enabled=True instead of latching a silent false-empty for the
handle's lifetime) had no direct coverage. Generalize the wrapper to
take an SQL needle — 'messages_fts LIMIT' hits the primary probe only,
since bare 'messages_fts' is a substring of the trigram table name —
and pin _fts_enabled staying True.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The discover payload echoes the query string, so asserting 'auth refactor' in the raw output proved nothing about matching — a zero-hit result would still pass. Assert on HIT-side fields instead: count >= 1, the hit's session_id, and the match snippet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ot the open tracker The three shim-feature cites in hermes_tools_mcp_server (module docstring, section banner, registration comment) pointed at NousResearch#26567 — the CLOSED issue about the scope docstring contradicting EXPOSED_TOOLS. The open tracking issue for exposing memory + session_search via stateless shims is NousResearch#26604 (already cited correctly at the fail-closed guard). Re-anchor all three, plus the same stale cite in the shim test file's header. Nothing in these spots references the docstring-mismatch history, so no NousResearch#26567 cite remains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cycle Resolves the NousResearch#26604 keep_open either/or as option (a). The hermes-tools MCP entry now names HERMES_SESSION_ID in env_vars — codex's spawn-time NAME passthrough — so the shim's own-lineage exclusion follows the ACTIVE session under codex instead of staying fail-open. The burn-in rule stands: no session VALUE is ever serialized into config.toml (the existing test pinning it passes untouched). End-to-end regression covers the leg the production-producer test deliberately scoped out: real producer writes the id, codex-style spawn snapshots exactly the names the real entry declares, the shim reads only what was delivered and excludes the calling lineage. Hosts that deliver nothing keep the documented fail-open behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s sqlite_safe_read's tracking retrofit Upstream's connect_tracked retags non-tracking connections by __class__ swap; the old object-proxy fake died in _retrofit_tracking with 'object layout differs from TrackedConnection'. Same fix the parity branch carries for its trigram-probe fake: a real sqlite3.Connection subclass injected via the factory kwarg. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…probe errors surface at open The rebase onto 7e47a0f adopted upstream's _fts_table_probe (cursor- based, raises on transient with close-on-failure). The fake only overrode connection-level execute, so probes never errored; and the keep-enabled transient pins tested semantics upstream has superseded — transient now surfaces at open with the tracked connection closed.
80fdedc to
91ae15a
Compare
SummaryThree PRs address or reference this issue complex: #26601 and #26603 correct the documentation contradiction in #26567, while #65978 implements the stateless memory/session_search capability tracked by #26604 and repairs read-only FTS probing. Related pull requests
Duplicates#26601 and #26603 substantially duplicate the doc-only correction for #26567; #26601 is closed as superseded by merged #26603. #65978 is not a duplicate because it implements the separate capability work tracked by #26604. Suggested consolidationKeep #65978 open with a salvage path: retain the stateless shims, read-only FTS correction, HERMES_SESSION_ID name-passthrough, and lifecycle regression, while asking the author to update the stale fail-open documentation and obtain contributor re-review of the previously blocking keep_open concern. Leave #26601 closed as a duplicate of #26603; #26603 remains the merged reference implementation for #26567. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I26567(["issue #26567 (closed)"])
I26604(["issue #26604 (open)"])
P65978["PR #65978 (open)"]
P65978 -.->|partial| I26567
P65978 -->|best fix| I26604
class I26567 closed
class I26604 open
class P65978 open
class P65978 best
class P65978 target
click I26567 "https://github.com/NousResearch/hermes-agent/issues/26567"
click I26604 "https://github.com/NousResearch/hermes-agent/issues/26604"
click P65978 "https://github.com/NousResearch/hermes-agent/pull/65978"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 65 kB of PR diffs, 13 kB of issue/PR text, 19 kB of discussion (11 comments), 7 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
… codex — reconcile the three stale fail-open passages with the env_vars name-passthrough Triage follow-up on NousResearch#26604/NousResearch#65978: the runtime page still described the pre-fix contract (session id never reaches the shim, exclusion inactive). The migration entry now names HERMES_SESSION_ID in env_vars, so exclusion follows the active session; fail-open remains only for hosts that deliver nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… 55 on this branch (claude_sdk_session_id, NousResearch#65978 lineage) vs 54 upstream
Summary
Read-only
SessionDBhandles silently returned[]fromsearch_messages()— the FTS availability probe latchedFalsefor the handle's lifetime on ANY error, including transient ones (database is lockedduring a checkpoint). This PR classifies transient vs absent for both the primary and trigram probes, and adds statelessmemory/session_searchMCP shims to the hermes-tools server (implements #26604) so runtimes that can't reach the agent-loop tools (codex app-server) regain both capabilities.Stacking
NousResearch#65982(claude-agent-sdk provider) is stacked on this PR — its branch contains these five commits at9756d26e5plus newer polish commits that ride this PR's head. Merge this first; #65982 restacks cleanly.Commits (10)
9756d26e5: RO probe classification,claude_sdk_session_idcolumn (consumer ships in feat(providers): claude-agent-sdk provider — the official Agent SDK as a first-class runtime under subscription OAuth (fail-closed) #65982), MCP shims + schema fix, session-id comment honesty,HERMES_MCP_SESSION_IDremoval (sweeper feedback — contract narrowed as documented).b1a78807e(2026-07-26): the codex-runtime docs page no longer claims memory/session_search are unavailable (it was made false by this PR); RO-probe comment states what the query path actually does; a transient-lock test now drives the PRIMARY probe (the old harness only intercepted the trigram probe); the seeded-rows shim test asserts hit-side fields (old assertion passed on zero hits); shim cites anchored to open tracker codex_app_server runtime: expose memory + session_search via stateless MCP shim #26604 instead of closed hermes_tools_mcp_server: scope docstring claims memory/session_search/delegate_task but EXPOSED_TOOLS excludes them #26567.Verification
Full targeted suite green (112 tests across shims/server/state); each commit buildable standalone;
uv lock --checkclean (no dependency changes).🤖 Generated with Claude Code