fix: validate agent_id before agent-runtime namespace concat (CLI + MCP)#491
Merged
fix: validate agent_id before agent-runtime namespace concat (CLI + MCP)#491
Conversation
Without a gate, hostile-shaped values like ``foo:bar`` or ``../x`` concatenate into ``AGENT_NAMESPACE_PREFIX`` and round-trip into storage as malformed namespace strings (``"agent-runtime:foo:bar"``). The CLI side mirrored the gap from the MCP path so fixing only one surface would leave the other open. Add ``validate_agent_id`` in ``constants.py`` (a thin wrapper around ``validate_name`` from ``context/_names.py`` with ``kind="agent-id"``) and apply it at the three entry points that build the namespace — ``mem_session_start`` (MCP), ``mm session start``, and ``mm session wrap`` (CLI). All three reject the same set (``:``, ``/``, ``..``, whitespace, control chars, and anything outside ``[A-Za-z0-9._-]``) with the same identifying error text; ``mm session wrap`` fails before the wrapped subprocess runs so an invalid id never leaves a half-set state file. Charset coverage stays in ``test_context_names``; new ``test_validate_agent_id`` pins the wiring at both surfaces and the regression contract: ``"agent-runtime:foo:bar"`` cannot reach storage from either entry point. Closes #486. Co-Authored-By: Claude <[email protected]>
Three small follow-ups from the PR #491 review: * ``validate_agent_id`` docstring previously claimed to gate "every boundary that concatenates AGENT_NAMESPACE_PREFIX". That overstated the scope — the LangGraph adapter's ``start_agent_session`` and ``MemtomemCheckpointer.namespace_for`` plus ``mem_agent_register`` / ``mem_agent_search`` still use the unvalidated / sanitize-instead-of-reject pattern. Narrow the docstring to the three surfaces actually wired in this PR and call out the deferred surfaces so future readers don't read the validator as load-bearing where it isn't. Follow-up parity work tracked separately. * ``HOSTILE_AGENT_IDS`` had a literal U+200B in source. Switched to the ```` escape so the test reads as intentional rather than as an editor artifact a future grep might "fix". * ``test_session_wrap_rejects_colon_before_subprocess`` only pinned the storage side of the contract — its docstring claims subprocess never runs, but it relied on storage_mock to catch the regression. Now patches ``subprocess.run`` directly with a MagicMock and asserts ``run_mock.assert_not_called()`` so the test matches its claim. Co-Authored-By: Claude <[email protected]>
This was referenced Apr 26, 2026
Collaborator
Author
|
Review fixes pushed in eaa9d3d:
Concern 5 ( Follow-up issues filed per the verdict:
|
memtomem
approved these changes
Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate_agent_idinconstants.py(thin wrapper aroundvalidate_name(value, kind="agent-id")).AGENT_NAMESPACE_PREFIXwith caller input:mem_session_start(MCP),mm session start, andmm session wrap(CLI).test_validate_agent_id:"agent-runtime:foo:bar"cannot reach storage from either surface.Why
PR #485 review (item 2) flagged that
f"{AGENT_NAMESPACE_PREFIX}{agent_id}"does no input validation. Hostile-shaped values (foo:bar,../x,spaces, control chars) round-trip into storage as malformed namespace strings. The CLI mirrored the gap from the MCP path so a single-surface fix would leave the other open.Single boundary, single charset, single error message:
[A-Za-z0-9._-](1–64 chars, no leading dash, not"."/".."). MCP path raises anInvalidNameError(aValueErrorsubclass) whichtool_handlerformats as"Error: invalid agent-id 'foo:bar': ..."; CLI path converts the same exception to aClickExceptionwith the identical core text.mm session wrapvalidates before the wrapped subprocess runs so an invalid agent_id never leaves a half-set.current_sessionfile or a rogue child.Out of scope
mem_agent_register/mem_agent_search(which stillsanitize_namespace_segmentrather thanvalidate_*); the issue scopes this PR to the session-start surfaces, and changing the multi_agent tools' silently-mutating behavior is a separate UX call.Test plan
uv run pytest -m "not ollama"— 2554 passed, 46 deselected (ollama).uv run ruff check+ruff format --checkonsrc/andtests/— clean.test_validate_agent_id:mem_session_start(agent_id="foo:bar")returns"Error: invalid agent-id ..."and storage stays emptymm session start --agent-id "foo:bar"exits non-zero, storage mock confirmscreate_sessionwas never calledmm session wraprejects before the wrapped command can run"invalid agent-id 'foo:bar'"core textCloses #486.
🤖 Generated with Claude Code