Skip to content

fix(mem0): api_key optional in schema; harden composition test env isolation - #64

Merged
Kyzcreig merged 1 commit into
mainfrom
fix/mem0-test-43
Jun 20, 2026
Merged

fix(mem0): api_key optional in schema; harden composition test env isolation#64
Kyzcreig merged 1 commit into
mainfrom
fix/mem0-test-43

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Summary

Two follow-up fixes for the real issues Greptile flagged on merged PR #43
(fix(blackbox): two-tier char→token divisor). Both were valid; neither was
addressed before that PR merged.

1. plugins/memory/mem0/__init__.pyapi_key wrongly required: True

is_available() bypasses api_key entirely when host is set — self-hosted
mode is gated on admin_api_key, not the cloud key:

def is_available(self) -> bool:
    cfg = _load_config()
    host = str(cfg.get("host", "") or "").strip()
    if host:
        return bool(str(cfg.get("admin_api_key", "") or "").strip())
    return bool(cfg.get("api_key"))

But get_config_schema() marked api_key required: True, so any
schema-driven setup/validation UI would block a self-hosted configuration
unless a cloud key it never uses was also supplied. Marked optional, with the
conditional requirement documented in the field description. is_available()
remains the real runtime gate, so cloud mode still effectively requires the key.

2. tests/test_request_composition.py — env leak on assertion failure

test_fixed_divisor_default_and_out_of_range_fallback mutated os.environ
directly without try/finally (unlike the adjacent monkeypatch tests). A
mid-loop assertion failure would leak HERMES_COMPOSITION_CHARS_PER_TOKEN_FIXED
into later tests, causing order-dependent failures. Switched to monkeypatch +
try/finally.

Tests

Behavior-contract tests added (not value snapshots):

  • api_key is not required in the schema.
  • self-hosted is_available() is true with host+admin_api_key, no api_key.
  • cloud mode still requires api_key.
tests/test_request_composition.py ............................ 28 passed
tests/plugins/memory/test_mem0_selfhost.py ............. 13 passed

Run in the repo venv with the worktree code on the path.

…olation

Two follow-up fixes from Greptile review of merged PR #43.

- plugins/memory/mem0/__init__.py: api_key was marked required:True even
  though is_available() bypasses it when host is set (self-hosted gates on
  admin_api_key). The required flag forced any setup/validation UI to demand
  a cloud key for a self-hosted server that never uses one. Mark it optional
  and document the conditional requirement; is_available() remains the real
  gate.
- tests/test_request_composition.py: test_fixed_divisor_default_and_out_of_range_fallback
  mutated os.environ directly without try/finally, so a mid-loop assertion
  failure would leak HERMES_COMPOSITION_CHARS_PER_TOKEN_FIXED into later tests.
  Switch to monkeypatch + try/finally, matching the adjacent override tests.

Adds behavior-contract tests: api_key not required in schema, self-hosted
is_available without an api_key, and cloud mode still requires one.
@github-actions

Copy link
Copy Markdown

🔎 Lint report: fix/mem0-test-43 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 11212 on HEAD, 11214 on base (✅ -2)

🆕 New issues (1):

Rule Count
invalid-assignment 1
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`

✅ Fixed issues (2):

Rule Count
unresolved-attribute 2
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
run_agent.py:2931: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`

Unchanged: 5855 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

CI note (not a blocker): the test (5) shard is RED on this PR, but it is a
pre-existing, unrelated infra timeout — not caused by this change.

  • The shard reports 4269 tests passed, 0 failed. The single red is
    tests/run_agent/test_run_agent.py hitting the 140.00s per-file timeout cap
    (a 6,619-line / 378-test file — the heaviest in the slice), reaching only ~10%
    before being killed: "1 file where no tests ran (timeout before collection)".
  • This PR's diff does not touch tests/run_agent/ or run_agent.
  • It reproduces identically on a second, independent PR off the same base
    (the sibling Greptile-fix branch) — same file, same 140.00s cap — confirming
    it's environmental shard-load/timeout, not either change.

All deterministic floor checks are green: sast, secret_scan, ruff enforcement, ruff + ty diff, e2e, check-attribution, osv-scanner,
supply-chain, and test shards 1–4 + 6.

@Kyzcreig
Kyzcreig marked this pull request as ready for review June 20, 2026 23:22
@Kyzcreig
Kyzcreig merged commit 2d9837b into main Jun 20, 2026
36 of 37 checks passed
@Kyzcreig
Kyzcreig deleted the fix/mem0-test-43 branch June 20, 2026 23:22
@greptile-apps

greptile-apps Bot commented Jun 20, 2026

Copy link
Copy Markdown

Greptile Summary

This PR delivers two targeted follow-up fixes from PR #43: correcting the api_key config schema entry from required: True to required: False (self-hosted mode gates on host+admin_api_key and never consults the cloud key), and hardening an env-leaking composition test by replacing raw os.environ mutation with monkeypatch + try/finally.

  • plugins/memory/mem0/__init__.py: api_key schema entry marked optional with a clarifying description; is_available() runtime logic is untouched, so cloud mode still effectively requires the key.
  • tests/plugins/memory/test_mem0_selfhost.py: Three new behavior-contract tests assert the schema change, self-hosted availability without a cloud key, and that cloud mode still demands one.
  • tests/test_request_composition.py: test_fixed_divisor_default_and_out_of_range_fallback now uses monkeypatch + try/finally so a mid-loop failure can no longer leak HERMES_COMPOSITION_CHARS_PER_TOKEN_FIXED into subsequent tests.

Confidence Score: 4/5

Safe to merge; the runtime availability logic is untouched and the schema change only affects validation UIs, not the actual cloud/self-hosted auth paths.

Both changes are narrow and well-tested. The schema fix correctly aligns the required flag with the actual runtime behaviour of is_available(). The test hardening eliminates a real order-dependent pollution risk with no functional side-effects. The only gap is a stale (required) annotation in the module-level docstring that contradicts the new schema entry.

The module docstring in plugins/memory/mem0/__init__.py still labels MEM0_API_KEY as (required) and should be updated to match the schema change.

Important Files Changed

Filename Overview
plugins/memory/mem0/init.py Single-line schema fix: api_key changed from required: True to required: False with an updated description. The runtime gate in is_available() is unchanged. Module-level docstring still annotates MEM0_API_KEY as (required), which is now stale.
tests/plugins/memory/test_mem0_selfhost.py Three new behavior-contract tests added: schema required assertion, self-hosted availability without api_key, and cloud mode still requiring api_key. All use monkeypatch + tmp_path isolation correctly.
tests/test_request_composition.py Converted test_fixed_divisor_default_and_out_of_range_fallback from raw os.environ mutation to monkeypatch + try/finally, eliminating the env-leak risk on mid-loop assertion failure. The cleanup logic is sound.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[is_available called] --> B{host set?}
    B -- Yes: self-hosted --> C{admin_api_key set?}
    C -- Yes --> D[✅ available]
    C -- No --> E[❌ not available]
    B -- No: cloud --> F{api_key set?}
    F -- Yes --> G[✅ available]
    F -- No --> H[❌ not available]

    subgraph Schema["get_config_schema() — after this PR"]
        S1["api_key: required=False ← changed"]
        S2["admin_api_key: required=False"]
        S3["host: optional"]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[is_available called] --> B{host set?}
    B -- Yes: self-hosted --> C{admin_api_key set?}
    C -- Yes --> D[✅ available]
    C -- No --> E[❌ not available]
    B -- No: cloud --> F{api_key set?}
    F -- Yes --> G[✅ available]
    F -- No --> H[❌ not available]

    subgraph Schema["get_config_schema() — after this PR"]
        S1["api_key: required=False ← changed"]
        S2["admin_api_key: required=False"]
        S3["host: optional"]
    end
Loading

Comments Outside Diff (1)

  1. plugins/memory/mem0/__init__.py, line 9 (link)

    P2 The module-level docstring still annotates MEM0_API_KEY as (required), but the PR changes it to required: False. A reader configuring self-hosted mode from the module header alone would still be misled into thinking the cloud key is mandatory.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "fix(mem0): api_key optional in schema; h..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant