Repository navigation
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
4a95f4a to
797dce5
Compare
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Approving at 797dce5, this PR's own three commits (1231881..797dce5). #1869's commit is covered in my review on #1869.
This reads well, and the commands now match what CI runs. One non-blocking nit:
maintainers/running-unit-tests.md, "Running Tests with a Keyword":pytest -k "create_memory"still runs from the root with no path, so it hits the same collection error inintegrations/strands-memmachine/tests/test_tool.py(it importsstrandsat module level) that this PR fixes in "Running All Tests".-konly filters after collection, so the import error still stops the run. Giving it the same two directories, e.g.pytest -k "create_memory" packages/server/server_tests packages/client/client_tests, keeps every command in that file runnable from the root.
Verified: the Checks section's Ruff, ty (--project per package) and pytest commands are the ones lint.yml and pytest.yml run, and those jobs skip drafts and re-run on ready_for_review as the guide says; -m integration overrides the addopts default; every relative link resolves (including the ts-client README's Development heading); nothing at head still references the removed files; and the dropped steps really are stale (no signature check in any workflow, no pre-commit or markdownlint config, docs/README.md has the current mint steps).
| ``` | ||
|
|
||
| All contributions, regardless of type, should follow this general workflow: | ||
| Integration tests are marked `integration` and start their databases in |
There was a problem hiding this comment.
Not every integration test gets its database from Docker. The NebulaGraph tests connect to an external server at NEBULA_HOST (default 127.0.0.1:9669, NebulaGraph Enterprise 5.0 or later) and skip when they can't reach it, and the Ollama-backed tests skip unless OLLAMA_HOST is set. A contributor who runs -m integration from this guide will see those skip and won't know why. Could you mention both variables here?
There was a problem hiding this comment.
Added in 783e3fc. The paragraph now says most integration tests start their databases in Docker and lists the ones that connect to a server you run, which skip when they can't reach it:
- NebulaGraph at
NEBULA_HOST, withNEBULA_USERandNEBULA_PASSWORD - an OpenAI-compatible server such as Ollama at
OLLAMA_HOST, which has no default - a MemMachine server at
MEMORY_BACKEND_URL, for the Python client's tests
🤖 Written by Claude Code (Claude Opus 5.5) on behalf of @edwinyyyu.
|
|
||
| - [ ] I have signed the commit(s) within this pull request | ||
| - [ ] My code follows the style guidelines of this project (See STYLE_GUIDE.md) | ||
| - [ ] My code follows the style guidelines of this project (See CONTRIBUTING.md) |
There was a problem hiding this comment.
CONTRIBUTING.md has no style guidelines now, only the Ruff, ty and pytest commands under Checks. The principles from STYLE_GUIDE.md and AGENTS.md (meaningful names, documenting public APIs, raise ... from err, catching broad exceptions only at API boundaries, the slow/integration markers) weren't moved anywhere. If Ruff and ty are meant to be the whole style guide, this checkbox could say so. Otherwise a short style section in CONTRIBUTING.md would give it something to point at.
There was a problem hiding this comment.
Added a Style section to CONTRIBUTING.md, so the checkbox has something to point at. It gives the rules Ruff and ty don't check, including spelled-out names, what docstrings hold, and the slow and integration markers. raise ... from err stays with Ruff, whose B904 rule requires it inside except blocks. The broad-exception rule from AGENTS.md is left out, because broad catches have legitimate uses away from API boundaries, such as cleanup that logs a secondary failure while the original error propagates. The section also says existing code may not match the rules, which doesn't exempt new code from them.
🤖 Written by Claude Code (Claude Opus 5.5) on behalf of @edwinyyyu.
|
Fixed the 🤖 Written by Claude Code (Claude Opus 5.5) on behalf of @edwinyyyu. |
CONTRIBUTING-CORE.md and CONTRIBUTING-DOCS.md each repeated the fork, branch, sign, and pull request steps of CONTRIBUTING.md. What remains of them fits in one file: the issue-first workflow, uv setup, the checks CI runs (with the test-only variables for the tests that call paid APIs), and links to docs/README.md for previewing the documentation site and to maintainers/running-unit-tests.md for selecting tests. The checks are the commands CI runs. From the repository root, `pytest` stops at a collection error in integrations/strands-memmachine, whose `strands` dependency no extra installs, and `ty check packages` reports the optional multihop group's spacy imports as unresolved. So the guide gives pytest the package test directories and type checks each package, and running-unit-tests.md's whole-suite command names the same directories. CONTRIBUTING-DOCS.md's Mintlify steps pointed at mint.json, which the repository doesn't have (it has docs/docs.json), and ran `mintlify install`, which the current CLI's README doesn't list. Its pre-commit hooks have no configuration in the repository. docs/README.md has the current `mint` steps. The claim that unsigned commits fail GitHub Actions is dropped: no workflow checks signatures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
Its commands and conventions repeat CONTRIBUTING.md, the Ruff and ty configuration, and the TypeScript client's README. It has gone without updates since March 2026, and its `uv run pytest` and `ty check packages` commands fail from the repository root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
Its tool guidance, Ruff and ty, is in CONTRIBUTING.md's Checks section. Its principles are generic, and its Markdown rules name a linter, markdownlint, that nothing runs. The pull request template's style checkbox points at CONTRIBUTING.md, and the documentation site's community page and .dockerignore drop their entries for the file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
Most integration tests start their databases in Docker. The NebulaGraph tests connect to NEBULA_HOST and skip when it is unreachable, the tests on an OpenAI-compatible server skip unless OLLAMA_HOST is set, and the Python client's tests skip without a MemMachine server at MEMORY_BACKEND_URL. The Checks section now names each one and its variables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
From the repository root, `pytest -k` with no path still collects integrations/strands-memmachine and stops at its import error, since -k filters only after collection. The example now names the server and client test directories, like the whole-suite command. Its keyword, create_memory, matched no test; add_memory matches eleven. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
The pull request template's style checkbox points at CONTRIBUTING.md, which held only the Ruff, ty, and pytest commands. The Style section gives the rules those tools don't check: spelled-out names and consistent function prefixes; no Any, cast, or object types and no suppression comments except where a third-party library forces them; public library APIs, async clients, and declared dependencies; broad exception handling at API boundaries, carried over from AGENTS.md; what comments and docstrings hold; when to factor out duplicated code or add a cache; and the integration and slow test markers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
Some existing code breaks the Style section's rules, and following the surrounding code would carry that into new code. The section now says the rules apply to new code regardless. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
Removed as too specific or not valid everywhere: the is_*/validate_* naming pair (functions that share a prefix still behave alike), no docstrings on @OverRide methods, comments that skip their callers, catching broad exceptions only at API boundaries, and no one-line helpers. The comments rule now keeps out references that can go out of date rather than every issue or pull request reference. Standard abbreviations such as id and url are allowed in names, and the Any, cast, object, and suppression exceptions cover Python's own API, such as `other: object` in __eq__, as well as third-party libraries'. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
774946d to
29c2d40
Compare
MemMachine#1706 added the README so that anyone can get up to speed with documentation work. Most of it copied Mintlify's own setup steps, and one of them had already drifted: it asked for Node 18 or higher, while the mint CLI refuses to start below 20.17 or on Node 25. Mintlify's installation guide states the current requirement and covers installing Node and the CLI, so the README links to it instead of copying it, which still serves the same readers. What stays is what only this repository decides: run `mint dev` from docs/, which the old wording called "the project root directory", and the checks docs CI runs there. Cloning is in CONTRIBUTING.md, and the CLI's other commands and options are in its own help. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Edwin Yu <edwinyyyu@gmail.com>
| a server you run, and skip when they can't reach it: | ||
|
|
||
| - **For Core Code Contributions**: If you'd like to contribute code, fix a bug, | ||
| or propose a new feature, please read our | ||
| [**Core Contribution Guide**](CONTRIBUTING-CORE.md). | ||
| - **For Documentation Contributions**: If you want to improve a typo, correct a | ||
| broken link, or add a new guide, please read our | ||
| [**Documentation Contribution Guide**](CONTRIBUTING-DOCS.md). | ||
| - NebulaGraph Enterprise 5.0 or later, at `NEBULA_HOST` (default | ||
| `127.0.0.1:9669`), with `NEBULA_USER` and `NEBULA_PASSWORD` | ||
| - An OpenAI-compatible server serving `qwen3:8b`, such as Ollama, at the | ||
| base URL in `OLLAMA_HOST`, which has no default |
There was a problem hiding this comment.
The Ollama tests don't skip when the server is unreachable. They skip only when OLLAMA_HOST is unset (packages/server/server_tests/memmachine_server/conftest.py:147-149), and the value goes straight to AsyncOpenAI as the base URL.
Ollama itself reads OLLAMA_HOST in host:port form (for example 127.0.0.1:11434, with no scheme and no /v1), so many Ollama users already have it set. For them, -m integration fails with connection or URL errors in the openai_chat_completions params of test_memmachine_integration.py and test_semantic_llm_integration.py instead of skipping. The same happens when the variable is set and the server is down.
Could this say the tests skip only when OLLAMA_HOST is unset, and give the expected format, e.g. http://localhost:11434/v1?
|
|
||
| ```bash | ||
| pytest | ||
| pytest packages/server/server_tests packages/client/client_tests |
There was a problem hiding this comment.
These commands use a bare pytest, but the Setup section linked above now only runs uv sync --all-extras. Since this PR removed the source .venv/bin/activate step, nothing puts the project's pytest on PATH. Someone following the guide gets command not found, or a system pytest without the workspace packages.
CONTRIBUTING.md uses uv run pytest. Could the commands here use it too, including the -k example further down?
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Re-reviewed at 4695aff. The keyword example from my first round is fixed, thanks. I checked Leo's two new threads against the code and agree with both, so I'll hold my re-approval until they're in:
-
CONTRIBUTING.md, Ollama bullet (Leo's thread on line 71):
openai_chat_completions_llm_configinpackages/server/server_tests/memmachine_server/conftest.pyskips only whenOLLAMA_HOSTis empty, so this bullet doesn't belong under "skip when they can't reach it". One more data point for the format line: where the value goes through the server's own config (theopenai_chat_completionscase oflanguage_model_configinmain/conftest.py), thebase_urlvalidator inlanguage_model_conf.pyrejects anything without a scheme, so Ollama's127.0.0.1:11434form fails at fixture setup with "Invalid base URL" rather than skipping. The repo's ownDEFAULT_OLLAMA_BASE_URLalready has thehttp://<host>:11434/v1shape, so Leo's example matches it. -
maintainers/running-unit-tests.md, bare
pytest(Leo's thread on line 18): the Prerequisites link used to point at CONTRIBUTING-CORE.md, which offered a plain venv withsource .venv/bin/activate; it now points at CONTRIBUTING.md#setup, whereuv sync --all-extrasis the only setup and nothing activates the venv.uv run pyteston all seven commands, the-kone included, makes the file match CONTRIBUTING.md.
Everything else from my last read still holds at this head: NebulaGraph skips on any connect failure, the client suite skips unless /api/v2/health answers at MEMORY_BACKEND_URL, leaving raise ... from err to Ruff holds (B selected, B904 not ignored), slow and integration are registered markers, and the docs/README.md commands are the ones docs-checks.yml runs. CI is green at head. Happy to approve once these two are in.
Purpose of the change
Fewer top-level files for contributors to read (#1652). The repository root loses
CONTRIBUTING-CORE.md,CONTRIBUTING-DOCS.md,STYLE_GUIDE.md, andAGENTS.md.CONTRIBUTING.mdbecomes the one contributing guide, and it links to the longer guides that already live in folders.Stack
CONTRIBUTING.md; removeAGENTS.mdandSTYLE_GUIDE.mdThis PR's nine commits, bfa1bab..4695aff, are on main after #1869's merge (377510d).
Description
CONTRIBUTING.mdsays once what the three contributing guides each repeated: the issue-first workflow, uv setup, the checks CI runs (Ruff, ty, and pytest), and the pull request steps. It links todocs/README.mdfor previewing the documentation site, tomaintainers/running-unit-tests.mdfor selecting tests, and to the TypeScript client's README for its commands.The checks are CI's commands. From the repository root,
pyteststops at a collection error, becauseintegrations/strands-memmachineneedsstrandsand no extra installs it.ty check packagesreports two unresolvedspacyimports from the optionalmultihopgroup. So the guide gives pytest the package test directories and type checks each package with--project, and the whole-suite and keyword commands inmaintainers/running-unit-tests.mdname the same directories. The keyword example'screate_memorymatched no test, so it now usesadd_memory.Integration tests with a server of your own: most integration tests start their databases in Docker. The Checks section lists the rest, which skip when they can't reach their server: NebulaGraph (
NEBULA_HOST,NEBULA_USER,NEBULA_PASSWORD), an OpenAI-compatible server such as Ollama (OLLAMA_HOST), and a MemMachine server for the Python client's tests (MEMORY_BACKEND_URL).Paid-API tests: the Checks section carries [contributor setup 1] Read paid-API test credentials from test-only variables #1869's text from
CONTRIBUTING-CORE.md. It lists theMEMMACHINE_TEST_*variables and warns that the tests call the real APIs, billed to those keys.Dropped as stale or unverified: three steps from
CONTRIBUTING-DOCS.md:mint.json: the repository hasdocs/docs.jsoninstead.mintlify install: the current CLI's README lists no such command.docs/README.mdlinks to Mintlify's installation guide for the current steps. Also dropped: the claim that unsigned commits fail GitHub Actions, since no workflow checks signatures. The signing requirement itself stays.Style: a Style section in
CONTRIBUTING.mdgives the rules Ruff and ty don't check: names, types, suppression comments, library use, comments and docstrings, factoring and caching, and test markers. It says existing code may not match them, which doesn't exempt new code.docs/README.md: Handoff files for Mintlify and Site skills #1706 added it so that anyone can get up to speed with documentation work. Most of it copied Mintlify's setup steps, and its Node requirement had drifted: it said 18 or higher, but themintCLI refuses to start below 20.17 or on Node 25. It now links to Mintlify's installation guide, which states the current requirement and covers installing Node, and keeps only what this repository decides: runmint devfromdocs/, and the checks docs CI runs there.STYLE_GUIDE.md: its Ruff and ty guidance is in the Checks section, and its Markdown rules named markdownlint, which nothing runs. The PR template's style checkbox now points atCONTRIBUTING.md, whose Style section replaces the file's principles, and the docs community page and.dockerignoredrop their entries for the file.AGENTS.md: its commands repeatCONTRIBUTING.mdand the TypeScript client's README. Of its conventions, Ruff and ty enforce the formatting, import, naming, and annotation rules. Its other preferences, such as pydantic models for structured data, small functions, pytest fixtures, and catching broad exceptions only at API boundaries, are not carried over.Fixes/Closes
Part of #1652. Two items stay open:
USAGE.md: [Docs]: Too many top-level repo files #1652's fix for it is a recommended usage path simple enough to fit in the README, which is a product change (related: memmachine-configure installs a JDK and Neo4j from downloaded archives #1866).alembic.ini: [Docs]: Too many top-level repo files #1652 defers it to Schema provisioning runs from every process's boot path: create_all races on cold boot and never evolves a table, Alembic runs destructive migrations on first use #1570, and [schema migration 1/2] Addmemmachine-server migrate, which creates every configured store's schema before servers start #1825 gives it one section per store lineage.Type of change
How Has This Been Tested?
Manual verification
At 4695aff: the new commit changes only
docs/README.mdand one line ofCONTRIBUTING.md. The README's three links return 200, and its commands are the onesdocs-checks.ymlruns indocs/. The Node bounds come from the CLI's own startup check (@mintlify/cli4.0.1789,bin/helpers.js), and Mintlify's installation guide states v20.17.0 or newer.At 29c2d40, rebased onto main after [contributor setup 1] Read paid-API test credentials from test-only variables #1869 merged: the PR's diff is identical to the previous head's (
git diff 862a08309 774946d01equalsgit diff 377510d43 29c2d4001), because main has changed none of the files this PR touches.git grepstill finds no reference to the removed files.At 774946d, after review: the five new commits change only
CONTRIBUTING.mdandmaintainers/running-unit-tests.md.uv run pytest -k add_memory packages/server/server_tests packages/client/client_testspasses (11 passed). The integration-test variables, defaults, and skip conditions match the NebulaGraph, Ollama, and client test fixtures.At 797dce5, rebased onto [contributor setup 1] Read paid-API test credentials from test-only variables #1869: the code is identical to [contributor setup 1] Read paid-API test credentials from test-only variables #1869's (
git diff 862a08309 797dce5f9 -- packages .github/workflowsis empty), so [contributor setup 1] Read paid-API test credentials from test-only variables #1869's checks cover it. Its documentation differs from the previous head, 4a95f4a, only in the integration-test paragraph.At d5a39a8, the previous base, after
uv sync --all-extras:ruff format --check,ruff check, and the threety check --projectcommands pass, anduv run pytest packages/server/server_tests packages/client/client_testspasses (2386 passed, 3 skipped).Every relative link and anchor in
CONTRIBUTING.mdresolves, and its external links return 200.git grepfinds no reference to the removed files.Test Results: as listed above.
Checklist
Further comments
Adding
testpathsto pytest's configuration would make a plainuv run pytestwork from the root again. That is a configuration change, so it is not in this PR.🤖 Written by Claude Code (Claude Opus 5.5) on behalf of @edwinyyyu.