Repository navigation
fix(agents): anchor agent-name validation against a trailing newline - #5960
Conversation
Two copies of the shared ^[A-Za-z0-9-]+$ grammar still used re.match,
whose $ also matches before a final newline, while every other copy uses
fullmatch. POST /api/agents {"name": "reviewer\n"} therefore passed the
router guard and died inside the strict file store as an HTTP 500, and
DeerMem accepted the same name as an agents/{name}/facts directory.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed against 08a797c. The fix itself is correct and complete: in Python, $ only matches before a single trailing \n (not \r/\v/\f/U+2028/U+2029, which the character class already excludes), so fullmatch closes exactly the remaining gap, and no previously-valid name changes behavior. Coverage is complete: all five agent-name routes funnel through _validate_agent_name, DeerMem's path builders all go through validate_agent_name, and every other copy of this grammar (agents_config, managed_subagents, channels/manager, scheduled_tasks, benchmark contract) already used fullmatch. Deferring the last .match in DeerFlowClient.__init__ to avoid #4313 conflicts is reasonable (I verified that PR rewrites client.py), but I'd like it tracked — see inline. Only two minor notes below.
| HTTPException: 422 if the name is invalid. | ||
| """ | ||
| if not AGENT_NAME_PATTERN.match(name): | ||
| if not AGENT_NAME_PATTERN.fullmatch(name): |
There was a problem hiding this comment.
Good fix — this closes the last .match on this grammar except one: backend/packages/harness/deerflow/client.py:233 (DeerFlowClient.__init__) still validates with .match, so a trailing-newline name slips past it there too. Deferring to avoid conflicts with #4313 (which rewrites that file) is reasonable, but the deferral currently lives only in the PR body — could you open/link a follow-up issue so the one-word change isn't lost after merge?
|
|
||
| @pytest.mark.parametrize("name", ["reviewer\n", "reviewer\n\n"]) | ||
| def test_trailing_newline_in_agent_name_is_rejected(self, agent_client, name): | ||
| """The router's ``AGENT_NAME_PATTERN.match`` accepted these and the store 500'd. |
There was a problem hiding this comment.
Minor accuracy nit: .match only accepted the "reviewer\n" param — "reviewer\n\n" was already rejected on main (Python's $ matches only before the single final newline), and the DeerMem test's "reviewer \n" param was too (the space is outside the class, so the match never reaches $). That matches your own main-branch run (2 of the 4 new params failed). Worth rewording so the docstring states the actual regression, e.g. "The router's AGENT_NAME_PATTERN.match accepted "reviewer\n" and the store 500'd" — otherwise a future reader may think \n\n needed this fix.
There was a problem hiding this comment.
Good catch — reworded in cd9c12b. Verified the claim directly: against ^[A-Za-z0-9-]+$, .match accepts only "reviewer\n"; "reviewer\n\n" and "reviewer \n" never reach $. Both docstrings now name the single param that regressed and say the other one is parametrized to pin the grammar rather than to report a second fix.
…ally covered .match only accepted "reviewer\n" on main; "reviewer\n\n" and "reviewer \n" were already rejected, so the docstrings should not read as if every param in these parametrizations was broken before the fix.
github-actions
Bot
added
area:agents
Why
Trigger. While reading the validators around the custom-agent routes I found the same agent-name grammar enforced two different ways.
config/agents_config.validate_agent_name,persistence/managed_subagents/base.py,app/channels/manager.pyandscheduled_tasks.pyall useAGENT_NAME_PATTERN.fullmatch(...), but_validate_agent_namein the Gateway router and DeerMem's inlined copy of the pattern still use.match(...). Python's$also matches before a trailing newline, so.match("^[A-Za-z0-9-]+$")accepts"reviewer\n". This is the same defect class #5911 (fix(paths): reject trailing newlines in scoped IDs) just fixed for the user / integration / project IDs — the two agent-name copies were left behind.Pain, measured on
main.POST /api/agents {"name": "reviewer\n"}passes the router's 422 guard, is normalized, and reachesFileAgentStore.create(), which validates the same grammar withfullmatchand raises:So the caller gets a 500 Internal Server Error — with the actionable message swallowed into the server log — for an input the API already rejects cleanly on every other route. On the memory side
validate_agent_name("reviewer\n")returnedNone, andagent_facts_directory()builtagents/reviewer\n/facts, a bucket the host's own strict validator refuses, so facts written there can never be read back through the agent APIs.What changed
POST /api/agents(andGET/PUT/DELETE /api/agents/{name},GET /agents/name/{name}) now answer 422 with the pattern message for a name that only differs by a trailing newline, instead of 500. Nothing that was previously a valid name changes:.matchand.fullmatchon this grammar differ only for strings ending in exactly one newline.validate_agent_name— and with itagent_facts_directory,agent_metadata_directory,agent_usage_path,agent_eviction_audit_path,fact_file_pathand theusers/*/agents/*scan instorage.py— rejects the same names.Deliberately out of scope:
DeerFlowClient.__init__has the same.matchcall on this pattern (client.py:233), but open PR #4313 rewrites the constructor lines immediately above it, so including it here would guarantee a conflict. Tracked as #6032 so the one-word change is not lost after merge.Surface area
backend/appBug fix verification
backend/tests/test_custom_agent.py::test_agent_name_validation_rejects_trailing_newline[reviewer\n]andbackend/tests/test_custom_agent.py::TestAgentsAPI::test_trailing_newline_in_agent_name_is_rejected[reviewer\n]mainand green on this branch? yes — onmainboth parametrisedreviewer\ncases fail (Failed … assert 500 == 422, and DeerMem's validator returns instead of raising); on this branchtests/test_custom_agent.pyis83 passed.Validation
uv run pytest tests/test_custom_agent.py→ 83 passed (onmainwith only the tests added: 2 failed, 81 passed)uv run pytest tests/test_memory_storage.py tests/test_deermem_self_contained.py tests/test_bench_deermem_scope_isolation.py tests/test_agents_router_model_settings.py tests/test_memory_cancel_by_agent.py tests/test_paths_user_isolation.py tests/blocking_io/test_agents_router.py tests/test_custom_agent.py→ 264 passeduv run pytest tests/test_memory*.py tests/test_bench_deermem*.py tests/test_harness_boundary.py→ 889 passed, 1 skippeduv run ruff format --check+uv run ruff checkon the three changed files (ruff 0.15.12) → cleanmake test/make lint(nomakeon this Windows host); the files above are the router's coverage plus every test module that imports the two validators this PR touches.AI assistance
Tool(s) used: Qoder CLI agent (Claude), running the
/contributorroutine autonomously.How you used it: the agent diffed the router's guard against the sibling validators that already use
fullmatch, measured the 500 traceback above onmain, wrote the failing tests first, then made the two one-word changes and re-ran the numbers quoted here. No human read the diff before submission, which is why the attestation box below is left unticked — it asserts that a person has read and takes responsibility for the change, and that person has not seen it yet.