Skip to content

fix(cli): the update check honours SURREAL_MEMORY_NO_UPDATE_CHECK, and the suite sets it - #215

Merged
acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/test-suite-writes-home-and-calls-pypi
Sep 7, 2026
Merged

acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/test-suite-writes-home-and-calls-pypi

Conversation

@RobertSigmundsson

Copy link
Copy Markdown
Contributor

Summary

  • run_update_check_background() gains an opt-out: it short-circuits when SURREAL_MEMORY_NO_UPDATE_CHECK is truthy.
  • tests/conftest.py sets it for the session, so a full test run makes no calls to PyPI.
  • Truthiness follows unified_config._env_truthy, the convention this repo already uses everywhere else.
  • Adds nineteen tests over the accepted spellings, the rejected ones, and the unset case.

Why

The update check had no opt-out at all. Most CLI invocations start a daemon thread that queries PyPI — the callback skips a short list of commands and --json, but not the several hundred invocations a full test run makes. On a machine with egress that is real outbound traffic from a unit test suite; on a machine without one it is the connection timeout instead.

It also feeds a flake, though not in the way it first appears. The banner goes to stderr, deliberately, with a comment in the code explaining that it must not break a piped smem recall … | jq. But CliRunner merges stderr into result.output, and the CLI tests parse that as JSON — so the banner lands in a different test's parse on each run.

Measured with strace -f -e trace=connect, counting connections to anything other than loopback, with a clean HOME so the 24-hour cache cannot mask the result:

tree SURREAL_MEMORY_NO_UPDATE_CHECK non-loopback connections
main unset 10
main 1 10 — the variable does not exist yet
this branch unset 10 — the check still works
this branch 1 0
this branch on, TRUE 0
this branch off, 0 10

The third row is the one that matters as a control: the opt-out suppresses the check, it does not break it.

Why this truthiness and not the obvious one

The obvious implementation — "any non-empty value disables it" — would give SURREAL_MEMORY_NO_UPDATE_CHECK=off the effect of switching the check off, which is the opposite of what anyone typing that would expect, and it would be a fourth convention in a repo that already has one.

So the gate calls unified_config._env_truthy, the same helper behind SURREAL_MEMORY_EMBEDDING_ENABLED, SURREAL_MEMORY_SYNC_ENABLED, SURREAL_MEMORY_SYNC_AUTO and the three SURREAL_MEMORY_REASONING_* switches: 1/true/yes/on, case-insensitive, and everything else — off, no, 0, false included — leaves the check running. The last two rows of the table above are that decision, measured.

Changes

  • cli/update_check.py: the short-circuit and a docstring recording the convention and why the test suite sets the variable.

  • tests/conftest.py: _isolated_home_dir sets SURREAL_MEMORY_NO_UPDATE_CHECK=1 alongside the $HOME redirect it already installs. Issue Test suite overwrites the user's real claude_desktop_config.json #110 and PR fix(tests): stop unit tests from writing into the real Claude Desktop config #121 closed the write side of that fixture; this closes the network side. A test that needs the real code path can monkeypatch.delenv it, and tests/unit/test_update_check_env_gate.py does exactly that.

  • tests/unit/test_update_check_env_gate.py (new).

  • docs/reference/config.md: regenerated with scripts/gen_config_docs.py, since that file is generated from these defaults and the Docs Freshness job checks it.

Test plan

  • pytest tests/unit/test_update_check_env_gate.py — 19 passed.
  • The strace table above: measured on both trees with a fresh HOME each time. The first attempt at this probe reported zero connections everywhere, because the check caches its answer for 24 hours in ~/.surrealmemory/.update_check and the real HOME had a warm cache — worth mentioning in case anyone reproduces it and wonders why nothing happens.
  • pytest tests/ -m "not stress" -n 4 against a live SurrealDB v3.2.0 — 7302 passed, 48 skipped, 1 xfailed, which is main's 7283 plus exactly the nineteen tests added here. Two tests in tests/unit/test_dashboard_brains_scope.py fail on this branch and on main alike: they want a live database and collide with one another under -n. Both pass when that file is run on its own.
  • ruff check src/ tests/ clean; ruff format --check src/ tests/ reports 740 files already formatted.
  • mypy src/ --ignore-missing-imports — success, no issues found in 354 source files.
  • Coverage under the CI gate: 72.30%, against 72.36% on main.
  • CHANGELOG.md untouched — left to the release entry, as with fix(storage): bind datetimes in time comparisons so they select by value #191fix(memory): refresh content-derived fields on compress, restore, and refine #193.

On the flake

The JSON-parsing flake in the CLI tests comes from the same banner reaching result.output through CliRunner, and setting the variable for the suite removes the banner. I am reporting that as a consequence rather than as this PR's proof: a green run is not by itself evidence that a flake is gone.

Verified by

@RobertSigmundsson

…d the suite sets it

run_update_check_background() had no opt-out. Most CLI invocations started a
daemon thread that queried PyPI — the callback skips a short list of commands
and --json, but not the several hundred invocations the test suite makes — so a
full run on a machine with egress made real outbound calls to pypi.org, and on
a machine without one paid the connection timeout instead.

It also feeds a flake. The banner itself goes to stderr, deliberately and with
a comment saying why, so it does not break a piped `smem recall … | jq`. But
click's CliRunner merges stderr into result.output, and the CLI tests parse
that as JSON — so the banner lands in a different test's parse each run.

The check now short-circuits when SURREAL_MEMORY_NO_UPDATE_CHECK is truthy,
and tests/conftest.py sets it alongside the $HOME redirect that acidkill#110 and acidkill#121
already put there. Same fixture, same reason: a test run should not reach out
of the machine it runs on.

Truthiness deliberately follows unified_config._env_truthy, the convention
this repo already uses for SURREAL_MEMORY_EMBEDDING_ENABLED,
SURREAL_MEMORY_SYNC_ENABLED and the SURREAL_MEMORY_REASONING_* switches:
1/true/yes/on, case-insensitive. Everything else — including off, no, 0 and
false — leaves the check running. A fourth, inverted convention where any
non-empty value disabled it would mean SURREAL_MEMORY_NO_UPDATE_CHECK=off
silently switching the check off, which is the opposite of what an operator
typing that would expect.

Nineteen tests in tests/unit/test_update_check_env_gate.py cover the accepted
spellings, the rejected ones, the unset case, and that a test needing the
real code path can still delete the variable.

@acidkill acidkill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff. Env gate follows the repo's canonical _env_truthy convention, conftest sets it session-wide, and falsy values (off/no/false) deliberately do not disable — an env var that looks like a switch never silently disables the check.

@acidkill
acidkill merged commit 2f078c6 into acidkill:main Sep 7, 2026
9 checks passed
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.

2 participants