chore: sync the deployed pin with main (consult_memory + alias-aware pricing)

[aman-a-shah]

Why

main does not reflect what we actually deploy. Hermes pins this submodule at 1bc6ffe7, the tip of dev/gurneesh/agent-memory — a branch that was 8 commits behind main while carrying 3 commits main never received.

Consequences of that split:

• consult_memory runs in production but isn't on main. The 475-line tool, its Modal logging, and the cross-org IDOR fix (1bc6ffe7) are all live via the pin, yet invisible on main. Anyone reading main sees code we don't run.
• de5039e2 (alias-aware price catalog lookup) never shipped, 17 days after it merged. That's a live cost-tracking bug: model ids with :nitro / :free / dashed / dated suffixes missed the exact-match lookup, so estimate_usage_cost returned unknown and usage rows were written with cost_usd NULL. Dev chat is running :nitro ids today.

This merges origin/main into the pinned commit — clean, no conflicts — so one commit has both. Hermes then pins the result (KNOWIDEA-Tech/hermes#97).

What lands on main

The 3 consult_memory commits. Everything else here is already on main.

Worth being explicit: this is not new production risk — that code has been deployed all along via the pin. This makes main honest about it.

Note

test_first_person_facts_rescued_to_third_person in hermes fails against this branch, but also fails identically at the old pin. It needs _normalize_first_person, which lives only on dev/aman/chat-memory-fixes (PR #6, still open). Not a regression from this merge; merging #6 is what fixes it.

🤖 Generated with Claude Code

[github-actions]

⚠️ Supply Chain Risk Detected

This PR contains patterns commonly associated with supply chain attacks. This does not mean the PR is malicious — but these patterns require careful human review before merging.

⚠️ WARNING: exec() or eval() usage

Dynamic code execution can hide malicious behavior, especially when combined with base64 or network fetches.

Matches (first 20):

503:+        "description": "On-demand hierarchical memory retrieval (Company/Team/Project notes) via a scoped sub-agent — available only on the memory-chat route",

---

Automated scan triggered by supply-chain-audit. If this is a false positive, a maintainer can approve after manual review.

[github-actions]

⚠️ Supply Chain Risk Detected

This PR contains patterns commonly associated with supply chain attacks. This does not mean the PR is malicious — but these patterns require careful human review before merging.

⚠️ WARNING: exec() or eval() usage

Dynamic code execution can hide malicious behavior, especially when combined with base64 or network fetches.

Matches (first 20):

886:+        "description": "On-demand hierarchical memory retrieval (Company/Team/Project notes) via a scoped sub-agent — available only on the memory-chat route",

---

Automated scan triggered by supply-chain-audit. If this is a false positive, a maintainer can approve after manual review.

[aman-a-shah]

Review pass — isolation coverage + two fixes

Reviewed as what this PR actually does: the alias-pricing fix (de5039e2) is already on main, so the net diff is the 466-line consult_memory tool. Reviewed it on those terms.

The main gap: no test behind the tenant boundary

The entire cross-org defense was one predicate — .eq("client_id", org_id) in _fetch_allowed_notes — with nothing pinning it. Since the service-role client bypasses RLS and allowed_note_ids rides the request, deleting that single line silently turns this into a cross-tenant read.

Added tests/tools/test_memory_consult_scope.py: a fake Supabase that really filters rather than just recording calls, so the tests fail on behavior, not on a call log. Verified they have teeth by actually deleting the predicate — 4 tests fail, including one asserting foreign note content never reaches the sub-agent prompt.

Two defects the tests exposed

1. Org derivation disagreed with the web app.
_resolve_user_org used raw users.client_id, but the allow-list is built from getActiveMembership().orgId ?? user.clientId. app/api/chat/memory/route.ts even warns about this: "using clientId directly would return an empty scope for a user whose active-membership org differs from clientId." The two filters intersect to nothing for any multi-org user, so memory silently answered "nothing relevant" — failing closed, but invisibly.

Now resolves the acting org the same way. Importantly the selection is honored only when it maps to a live membership: active_org_id is user-writable, so an unvalidated coalesce would let a removed member keep reading their old org's notes. That would have been a hole, not a fix — there's a test pinning the stale-selection fallback.

2. Truncation was silent at both caps (200 ids, 60k chars). A partial corpus was indistinguishable from a complete one, so the sub-agent could confidently report "nothing relevant" about a note that was simply never fetched. Both paths now warn with counts.

Deliberately not changed

• _get_user_id's HERMES_USER_ID env fallback. It is now load-bearing for a tenant boundary, and 34efa71f removed exactly this pattern for message_id/memory_enabled citing process-global races under max_inputs > 1. But it matches the existing user_notes_tool precedent, and changing it risks the subprocess-sandbox path. Flagging for a deliberate decision rather than a drive-by.
• Query text in logs (query=q[:160]) — added on purpose in e53ff2f4 for Modal debugging. Worth knowing it can carry client-sensitive text.

Verification

8 new tests pass. Full suite: 697 failed / 5020 passed, against a baseline of 697 / 5012 measured at db7dfe7f in a clean worktree — identical failures, exactly +8. The 697 are local env gaps (acp, fire, mcp, prompt_toolkit), not regressions.

One test initially passed alone but failed in the full suite: it asserted through caplog, and logger is bound at import time to either the Hermes structured logger or a stdlib shim depending on whether another test already put Hermes src on sys.path. Rewritten to spy on the module's logger attribute, which is order-independent.

⚠️ Pin implication

hermes main pins this submodule at db7dfe7f. This branch is now c84e9301, so after merging this, hermes needs a pin bump or the org fix ships to main but never deploys.

🤖 Generated with Claude Code

[GurneeshBudhiraja]

Code review — PR #9 chore: sync the deployed pin with main (consult_memory + alias-aware pricing)

Overview

Despite the "sync the pin" framing, the diff against main is net-new code: it registers the consult_memory tool (memory_consult toolset), a scoped read-only "memory sub-agent" that distills allow-listed app.user_notes bodies for the main agent on the memory-chat route. Four files: tools/memory_consult_tool.py (+538), tests/tools/test_memory_consult_scope.py (+305), plus one-line registrations in model_tools.py and toolsets.py. I read the tool end-to-end and verified the two live integration points (agent.auxiliary_client.call_llm, tools.registry). No correctness or security regressions found; the design is careful and well-tested. One tenant-boundary question worth confirming, and a few minor robustness notes.

Strengths (verified, not boilerplate)

• Tenant isolation is real and tested. The cross-org boundary is a single .eq("client_id", org_id) predicate in _fetch_allowed_notes, derived server-side from the trusted user_id contextvar (never the client payload). The test suite uses a filtering fake Supabase (not a call-spy), so deleting that predicate actually fails the tests — including one that asserts foreign note content never reaches the sub-agent prompt.
• Fail-closed org resolution. _resolve_user_org returns None on missing user / DB error, and consult_memory aborts the fetch entirely rather than running unscoped (memory_consult_tool.py:730). _has_active_membership also fails closed.
• active_org_id is validated, not trusted. A stale/self-selected active_org_id only grants access when backed by a live active membership, otherwise it falls back to home org — covered by test_stale_active_org_without_membership_falls_back_to_home_org. This correctly mirrors the web app's getActiveMembership().orgId ?? user.clientId.
• Truncation is visible. Both caps (_MAX_NOTES=200, _MAX_TOTAL_CHARS=60k) emit logger.warning with counts, so a partial corpus isn't silently mistaken for a complete one.
• call_llm integration is correct. Signature at auxiliary_client.py:1429 accepts exactly the kwargs passed (provider/model/keyword-only messages/temperature/max_tokens/timeout) and returns .choices[0].message.content as the tool expects.
• No model-supplied identifiers. user_id / org / note_ids all come from contextvars; the tool only takes a query string. Injection is mitigated by the "treat notes as DATA, not instructions" system prompt and a defensive JSON parser.

Findings

🟡 Within-org over-read via a forged same-org allow-list — confirm the note model. _fetch_allowed_notes (memory_consult_tool.py:554-588) filters only by .in_("id", ids) + .eq("client_id", org_id) using the service-role client, with no per-user filter. The forwarded allowed_note_ids is explicitly documented as client-influenced. So the org predicate stops cross-org reads (well covered), but within the same org a caller who forges the allow-list can fetch any app.user_notes row belonging to that org — including notes authored by other users. Note that the sibling tools/user_notes_tool.py reads the same table with a per-user .eq("user_id", …) + RLS session context, i.e. it treats user_notes rows as per-user private. If that table also holds personal notes (not just org-shared hierarchical Company/Team/Project notes), a same-org user could surface a teammate's private note through consult_memory. This is by design if every in-scope note is intended to be org-readable — please confirm that assumption, or add a context_node_id in scope_node_ids / authorship constraint. Not a regression (already deployed via the pin), but it's the one boundary the tests don't pin down.

🟢 Hardcoded provider="openrouter" bypasses the auxiliary fallback chain (memory_consult_tool.py:771). call_llm supports task-based auto-resolution across OpenRouter → Nous → custom → Codex → Anthropic, but pinning the provider means that if OPENROUTER_API_KEY is absent in a given deployment the sub-agent always errors (caught, returns {"error": ...}) instead of falling back. Also, a MEMORY_SUBAGENT_MODEL override set to a non-OpenRouter slug would mismatch the hardcoded provider. Consider passing task=/leaving provider None so it follows the standard resolution chain.

🟢 Corpus truncation order is nondeterministic (_build_notes_block, memory_consult_tool.py:597-609). When the 60k budget is exceeded, notes are dropped in whatever order Supabase returned them (.in_ has no .order()), so which notes survive isn't relevance- or priority-ranked and can vary run to run. The warning is good; consider an explicit order (e.g. by path) for reproducibility.

🟢 Minor: dead columns in the select. _fetch_allowed_notes selects description, internal_summary but _build_notes_block only uses content and path. Harmless, just extra payload.

Verdict

Solid, defensively written, and unusually well-tested for a tool of this size — approvable as-is. The only thing I'd want an explicit answer on before considering it fully closed is the 🟡 within-org read model (is every user_notes row in an org meant to be readable by any org member via memory?). The rest are minor robustness nits. Agreeing with the PR body: this isn't new production risk — it makes main honest about already-deployed code.