diff --git a/emrg/server/daemon.py b/emrg/server/daemon.py index 76198d11..7da8a308 100644 --- a/emrg/server/daemon.py +++ b/emrg/server/daemon.py @@ -93,7 +93,12 @@ ServerPong, TaskRequest, ) -from emrg.session import Session, last_n_messages, records_to_messages +from emrg.session import ( + Session, + is_safe_session_id, + last_n_messages, + records_to_messages, +) # ── 会话级 sandbox 档位（rant 2026-09-30T09:30:16）───────────────────── @@ -1570,6 +1575,12 @@ async def _handle_client(self, ws) -> None: "error": "task requires session_id and cwd", }) continue + # This branch is the one door `_process_message` does not own, and it + # is the door that creates the session directory: without this call a + # `task` carrying an unsafe id reached `_get_or_create_session` and + # wrote outside `/.emrg/sessions` (measured on this branch). + if await self._refuse_an_unsafe_session_id(ws, "task", session_id): + continue # Phase 2 session-level lock (protocol-contract §2.6.5): # one active task per session — concurrent tasks queue. # P1 (rant 21:55:37): construct req + allow_tools FIRST @@ -2783,12 +2794,52 @@ def _collect_history_data(self, session: Session) -> dict[str, str]: "date_str": datetime.now().strftime("%y%m%d"), } + async def _refuse_an_unsafe_session_id( + self, ws, msg_type: str, session_id: object + ) -> bool: + """Refuse a client-supplied session id that is not one path component. + + True means the id was refused and the client has been told, so the caller stops. + + A session id arrives in a client message and lands on a path in half the handlers + below: `_get_or_create_session` (eleven callers) writes it, the resume reader and + the two memory readers open it, and `delete_session` hands the resolved path to + `shutil.rmtree`. Measured 2026-10-21: `Session.delete("../..", cwd)` walked + `cwd/.emrg/sessions/../..` and left the project tree empty. + + **Two dispatchers, not one.** `_handle_client` intercepts `{"type": "task"}` + itself and never reaches `_process_message`, so a check placed only in the latter + left the one message type that *creates* a session directory unguarded — measured + on this branch before the fix: a `task` carrying `../../../evil` created a + directory one level above the project root and started its turn, telling the + client nothing. Both call sites ask this method, so the rule has one writer even + though it has two doors. + """ + if not session_id or is_safe_session_id(session_id): + return False + logger.warning( + "refused a %r message carrying session id %r: a session id is one " + "path component inside /.emrg/sessions", msg_type, session_id, + ) + await self._send(ws, { + "error": f"invalid session id: {session_id!r} (a session id is " + f"one path component)", + }) + return True + async def _process_message( self, msg: dict, ws ) -> None: """Process a single message and send responses.""" msg_type = msg.get("type", "") + # Every message type dispatched below takes its session id from this message and + # most of them paste it onto `/.emrg/sessions/`. Checked here, before any + # handler builds a path; `{"type": "task"}` is checked in `_handle_client`, + # which never reaches this method. + if await self._refuse_an_unsafe_session_id(ws, msg_type, msg.get("session_id")): + return + if msg_type == "ping": elapsed = int( (datetime.now() - self.start_time).total_seconds() diff --git a/emrg/server/prompts/system.j2 b/emrg/server/prompts/system.j2 index eb25f85e..854e6f3e 100644 --- a/emrg/server/prompts/system.j2 +++ b/emrg/server/prompts/system.j2 @@ -37,7 +37,7 @@ Each skill is a file of instructions. When one bears on what the host asked, rea - **Every call carries an `intent`**: one plain sentence on why it is made and what it should reach. It is never executed, but the host reads it, so it must be true and specific. - **Read a file before you change it**, so the change matches what is there. -- **Issue every call you can already specify, in one response.** Sending several is the normal case: a call goes alone only when its arguments need an answer you have not read yet, and then its `intent` names the specific answer it waits for. Every round re-sends the conversation and emits fresh output. +- **Plan the round before the first call.** Enumerate what this round has to establish and the readings it needs, and write that enumeration out as the opening lines of your reply, before you call anything. Every reading you can already specify goes out in one response; a call stands alone only when its arguments need an answer you have not read yet. The plan is revised between rounds, when a reading changes what the round can still settle — never between two calls of the same round. One round re-sends the whole conversation and emits fresh output, so a lone call buys a single reading at the price of a round. ## Memory diff --git a/emrg/session.py b/emrg/session.py index e315170e..6654532a 100644 --- a/emrg/session.py +++ b/emrg/session.py @@ -16,6 +16,7 @@ import hashlib import json import logging +import ntpath import os import secrets from datetime import datetime @@ -306,6 +307,39 @@ def _refuse_a_non_regular_subject(path: Path) -> None: ) +#: Characters that make a name more than one path component, on either platform named +#: here: `/` separates on POSIX and Windows, `\` separates on Windows and is an ordinary +#: character on POSIX — refusing both is the point, because the same id reaches a path on +#: whichever platform runs it. +_NAME_SEPARATORS = ("/", "\\") + + +def is_safe_session_id(session_id: object) -> bool: + """True when `session_id` names exactly one entry inside the sessions directory. + + A session id arrives in a client message, so it is an input like any other: + `Path(cwd) / ".emrg" / "sessions" / session_id` hands `..` and an absolute id to the + kernel, which resolves them, and the caller then removes what it resolved to. Measured + 2026-10-21: `Session.delete("../..", cwd)` walked `cwd/.emrg/sessions/../..` — the + project tree — and left it empty, `victim/` and `.emrg/` alike, then raised from + `shutil.rmtree`'s final `os.rmdir`. + + So the id is checked before a path is built: non-empty, no NUL, not `.` or `..`, no + separator, and no drive. The drive is asked of **both** flavours — `os.path.splitdrive` + answers `("", "C:evil")` on this host and `ntpath.splitdrive` answers `("C:", "evil")`, + and a `C:`-relative id is a path, not a name, on the platform that can receive it. + """ + if not isinstance(session_id, str) or not session_id: + return False + if "\0" in session_id or session_id in (".", ".."): + return False + if any(sep in session_id for sep in _NAME_SEPARATORS): + return False + if os.path.splitdrive(session_id)[0] or ntpath.splitdrive(session_id)[0]: + return False + return True + + class Session: """Manages a single conversation session on disk.""" @@ -981,10 +1015,20 @@ def clear(self) -> None: def delete(session_id: str, cwd: Path) -> bool: """Delete a session directory and all its contents. - Returns True if the session was deleted, False if it didn't exist. + Returns True if the session was deleted, False if it didn't exist — and False, + with a warning, for an id that is not a single path component, because the id + comes from a client message and `shutil.rmtree` follows whatever it resolves to. """ import shutil + if not is_safe_session_id(session_id): + logger.warning( + "refusing to delete a session named %r: a session id is one path " + "component, so nothing outside the sessions directory is reachable", + session_id, + ) + return False + session_dir = cwd / ".emrg" / "sessions" / session_id if session_dir.exists(): shutil.rmtree(session_dir) diff --git a/tests/test_daemon.py b/tests/test_daemon.py index 9d6838d7..fe4d9b7f 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -5622,3 +5622,143 @@ def test_an_out_of_domain_limit_is_refused_rather_than_coerced(tmp_path): assert frames[0]["type"] == "input_history" assert "integer limit and offset" in frames[0]["error"] + + +# ── a session id is one path component, at both doors a message can use ──────── +# +# The id is pasted onto `/.emrg/sessions/` by eleven `_get_or_create_session` +# call sites, by the resume reader, by the two memory readers and by +# `delete_session`, which hands the resolved path to `shutil.rmtree`. So the check +# lives once, in `_refuse_an_unsafe_session_id`: measured 2026-10-21, an id of +# `../..` reached `rmtree` with the project tree. +# +# It has **two** callers because the daemon has two dispatchers. `_handle_client` +# intercepts `{"type": "task"}` itself and never reaches `_process_message`, so a +# check placed only in the latter left unguarded the one message type that creates +# a session directory — measured on this branch before the fix: a `task` carrying +# `../../../evil` created a directory one level above the project root, started its +# turn, and sent the client nothing but `auth_ok`. A test that hands a `task` frame +# to `_process_message` therefore certifies a door production does not use: that +# function has no `task` branch at all and answers `unknown message type`. The two +# tests below drive `_handle_client`, which is the door. + + +def _project_with_a_neighbour(tmp_path): + (tmp_path / ".emrg" / "sessions" / "keep").mkdir(parents=True) + (tmp_path / "victim").mkdir() + (tmp_path / "victim" / "precious.txt").write_text("keep me", encoding="utf-8") + return tmp_path + + +class _FakeSocket: + """A websocket stand-in the read loop can drive: `recv` yields frames, then ends.""" + + def __init__(self, frames: list[dict]) -> None: + self._frames = [json.dumps(f) for f in frames] + self.sent: list[dict] = [] + + async def recv(self) -> str: + if not self._frames: + from websockets.exceptions import ConnectionClosed + raise ConnectionClosed(None, None) + return self._frames.pop(0) + + async def send(self, data) -> None: + self.sent.append(json.loads(data)) + + async def close(self) -> None: + pass + + +def _drive_read_loop(monkeypatch, tmp_path, frames: list[dict]) -> list[dict]: + """One real `_handle_client` connection: auth, then `frames`, then a disconnect. + + `_auth_token` is set by `EmrgServer.run()` when it binds a socket, and a unit + test never binds one, so it is pinned here. The two planted-fire markers are + pointed at `tmp_path` because a `task` frame really does start a tool loop, and + the loop stamps host state at round finalization (issue #1337). + """ + monkeypatch.setattr(daemon_mod, "_PLANTED_FIRE_MARKER_PATH", tmp_path / "planted-fire") + monkeypatch.setattr(daemon_mod, "_PLANTED_FIRE_ROUND_COMPLETE_PATH", + tmp_path / "planted-fire-round") + server = _make_server() + server._auth_token = "test-token" + socket = _FakeSocket([{"type": "auth", "token": "test-token"}, *frames]) + asyncio.run(server._handle_client(socket)) + return socket.sent + + +def test_delete_session_refuses_an_id_that_is_not_one_component(tmp_path): + """A refused delete leaves the project alone and says so in a frame.""" + project = _project_with_a_neighbour(tmp_path) + server = _make_server() + + frames = _drive(server, { + "type": "delete_session", "session_id": "../..", "cwd": str(project), + }) + + assert len(frames) == 1 + assert "invalid session id" in frames[0]["error"] + assert (project / "victim" / "precious.txt").read_text(encoding="utf-8") == "keep me" + assert (project / ".emrg" / "sessions" / "keep").is_dir() + + +def test_delete_session_still_deletes_a_real_one(tmp_path): + """The other direction on the same seam: a plain id is untouched by the refusal.""" + project = _project_with_a_neighbour(tmp_path) + server = _make_server() + + frames = _drive(server, { + "type": "delete_session", "session_id": "keep", "cwd": str(project), + }) + + assert frames[0]["type"] == "session_deleted" + assert frames[0].get("error") is None + assert not (project / ".emrg" / "sessions" / "keep").exists() + assert (project / "victim" / "precious.txt").exists() + + +def test_the_read_loop_refuses_a_task_carrying_an_unsafe_session_id( + tmp_path, monkeypatch +): + """`_get_or_create_session` writes the directory, so the refusal precedes it. + + Driven through `_handle_client`, because that is where a `task` is dispatched. + The id walks two levels up from the sessions directory, so without the refusal + the directory it creates lands **outside** the project — which is what the + assertion on `tmp_path` measures, not on `sessions/`. + """ + project = tmp_path / "project" + (project / ".emrg" / "sessions" / "keep").mkdir(parents=True) + + frames = _drive_read_loop(monkeypatch, tmp_path, [{ + "type": "task", "id": "t1", "session_id": "../../../evil", + "cwd": str(project), "prompt": "hi", + }]) + + assert [f for f in frames if f.get("type") != "auth_ok"] == [ + {"error": "invalid session id: '../../../evil' (a session id is one path component)"} + ] + assert not (tmp_path / "evil").exists() + assert sorted(p.name for p in (project / ".emrg" / "sessions").iterdir()) == ["keep"] + + +def test_the_read_loop_still_accepts_a_task_carrying_a_plain_session_id( + tmp_path, monkeypatch +): + """The other direction at the same door: a plain id creates its own session and starts. + + The turn's LLM call cannot be reached in a unit test, so the assertion is the one + that discriminates: no `invalid session id` frame, and the session directory the + branch creates exists. + """ + project = tmp_path / "project" + (project / ".emrg" / "sessions" / "keep").mkdir(parents=True) + + frames = _drive_read_loop(monkeypatch, tmp_path, [{ + "type": "task", "id": "t2", "session_id": "fresh", + "cwd": str(project), "prompt": "hi", + }]) + + assert not [f for f in frames if "invalid session id" in str(f.get("error", ""))] + assert (project / ".emrg" / "sessions" / "fresh").is_dir() diff --git a/tests/test_evolution_prompt_round_cost.py b/tests/test_evolution_prompt_round_cost.py index 3a548cbb..948bc8c8 100644 --- a/tests/test_evolution_prompt_round_cost.py +++ b/tests/test_evolution_prompt_round_cost.py @@ -72,21 +72,28 @@ ), } -#: The rule's unit, its criterion and its boundary, as shipped in **both** carriers. Editing either +#: The rule's unit, its criterion and its reason, as shipped in **both** carriers. Editing either #: one alone is the defect this pair catches, so `test_both_carriers_state_the_same_unit_and_criterion` #: reads the live text of each rather than one file's expectations about the other. #: #: These are the longest phrases the two carriers genuinely share, and that is a measurement rather #: than a preference: R10 is a rulebook entry a cycle reads and `system.j2` is one bullet in every -#: session's prefix, so the two state the same rule at different lengths — `issue every call you can -#: already specify, in one response: sending several tool calls in one round … is the normal case` -#: against `**Issue every call you can already specify, in one response.** Sending several is the -#: normal case: …`. The phrases below are where they agree word for word (the rule's direction, -#: the boundary that decides a lone call, and what that lone call must still say), and a drift in -#: any of them still moves both — which is the property the comparison exists for. -SHARED_UNIT = "every call you can already specify" +#: session's prefix, so the two state the same rule at different lengths — R10's `issue every call you +#: can already specify, in one response: sending several tool calls in one round … is the normal case` +#: against the bullet's `Every reading you can already specify goes out in one response; a call stands +#: alone only when its arguments need an answer you have not read yet`. +#: +#: The marker set moved on 2026-10-21 (issue #2437), when the bullet's subject changed from a count of +#: calls to a plan of the round, and it moved because a marker that one side dropped cannot be a phrase +#: two carriers share. What went is `names the specific answer it waits for`: the rewritten bullet does +#: not restate it, so that clause now lives in R10 alone, and by that alone it is not a shared phrase — +#: a comparison holding it would report every future rewrite as a drift. What came in is the reason +#: (`emits fresh output`), which both carriers do still state word for word. The ruled-out alternative +#: was to keep the old marker and edit R10 in the same PR: the host scoped this change to the bullet, +#: and a rulebook entry rewritten to fit a one-line prompt change is the drift this test exists to stop. +SHARED_UNIT = "you can already specify" SHARED_CRITERION = "its arguments need an answer you have not read yet" -SHARED_BOUNDARY = "names the specific answer it waits for" +SHARED_REASON = "emits fresh output" #: The unit this rule may no longer be stated in. The first version of R10 taught exactly #: these; the host retired the merge-several-calls-into-one reading explicitly at @@ -185,7 +192,7 @@ def test_both_carriers_state_the_same_unit_and_criterion() -> None: "emrg/server/prompts/system.j2": SYSTEM_PROMPT.read_text(encoding="utf-8"), } for marker, label in ((SHARED_UNIT, "unit"), (SHARED_CRITERION, "criterion"), - (SHARED_BOUNDARY, "boundary")): + (SHARED_REASON, "reason")): missing = [name for name, text in carriers.items() if marker not in text] assert not missing, ( f"the {label} `{marker}` is not in {missing}: R10 and system.j2 carry one rule, so " diff --git a/tests/test_session_delete_path.py b/tests/test_session_delete_path.py new file mode 100644 index 00000000..63f84eda --- /dev/null +++ b/tests/test_session_delete_path.py @@ -0,0 +1,75 @@ +"""A session id is one path component, and the delete path is where it must hold. + +A session id arrives in a client message and is pasted onto +`/.emrg/sessions/` before anything opens, reads or removes it. The kernel +resolves `..` and an absolute id at that point, so an id of `../..` names the +project tree rather than a session directory inside it. + +Measured 2026-10-21 through `Session.delete("../..", )`: the call walked +`/.emrg/sessions/../..`, left the project tree empty — a `victim/` +directory and `.emrg/` alike — and then raised from `shutil.rmtree`'s final +`os.rmdir` on the unresolved path. These tests pin the refusal and, in the other +direction, that a real id still deletes its own directory. +""" + +import pytest + +from emrg.session import Session, is_safe_session_id + + +@pytest.mark.parametrize("session_id", [ + "emrg-task", + "s_260727_1103_866d", + "a", + "sess-1.2", + "会话一", +]) +def test_a_plain_name_is_one_component(session_id): + """Every id this daemon generates, and a client-chosen name beside them.""" + assert is_safe_session_id(session_id) is True + + +@pytest.mark.parametrize("session_id", [ + "", # nothing to name + ".", # the sessions directory itself + "..", # its parent + "../..", # the project tree + "a/b", # a path, not a name + "/abs", # an absolute path replaces the join entirely + "a\\b", # a separator on Windows, an ordinary character on POSIX + "..\\..", + "C:evil", # a drive-relative path: one component nowhere + "x\0y", # a name no filesystem accepts + None, + 3, +]) +def test_a_name_that_is_not_one_component_is_refused(session_id): + assert is_safe_session_id(session_id) is False + + +def _project(tmp_path): + """A project tree: one session directory, and a neighbour that must survive.""" + (tmp_path / ".emrg" / "sessions" / "keep").mkdir(parents=True) + (tmp_path / "victim").mkdir() + (tmp_path / "victim" / "precious.txt").write_text("keep me", encoding="utf-8") + return tmp_path + + +def test_an_escaping_id_deletes_nothing(tmp_path): + """The measured shape: the id resolves to the project tree, so nothing may move.""" + project = _project(tmp_path) + + assert Session.delete("../..", project) is False + + assert (project / "victim" / "precious.txt").read_text(encoding="utf-8") == "keep me" + assert (project / ".emrg" / "sessions" / "keep").is_dir() + + +def test_a_real_id_still_deletes_its_own_directory(tmp_path): + """The other direction: the refusal must not cost the feature.""" + project = _project(tmp_path) + + assert Session.delete("keep", project) is True + + assert not (project / ".emrg" / "sessions" / "keep").exists() + assert (project / "victim" / "precious.txt").exists() diff --git a/tests/test_several_calls_rule_reach.py b/tests/test_several_calls_rule_reach.py index 65c63376..af645cde 100644 --- a/tests/test_several_calls_rule_reach.py +++ b/tests/test_several_calls_rule_reach.py @@ -37,15 +37,26 @@ itself. That is still a placement and wording guard: the share is re-measured by the instrument, over the window a later cycle names, never here. +The bullet's subject changed on 2026-10-21 (issue #2437), because the count was the wrong subject: it +left a reader free to meet the rule one call at a time, since its only test was whether a call "needed" +an answer. The rule now asks for a **plan of the round, formed before the first call, and written out as +the opening lines of the reply** — the act, not the attitude. Two guards carry that: `RULE_TERMS` holds +each part of the ruling, and `RULE_ACTION_PHRASES` holds the two acts separately, one test case each, so +deleting either one reddens a test that names it. Also measured here on the day of the change: **9.7%** +of 137,592 rounds carry two or more calls, and 92.5% of the lone ones carry an `intent` naming no awaited +answer — the same instrument, re-read, because a wording change is cheap to claim. + What is asserted ---------------- 1. The rule is in `system.j2` **and in the render**, and once only: a block in the file that no render contains is not a rule any session runs under, and a second copy is two copies free to drift. -2. It carries each part of the ruling — the unit (one round, several calls), the boundary (a call whose - arguments depend on another's answer waits for its own round), the reason (cost, not speed), and the - precedence of the no-background-process red line. -3. The three misstatements are **gone**: no `parallel`, no `for speed`, no 并发. One statement, not two. -4. **Reverse**: no wording that caps, budgets or quotas *rounds* may enter the shared prompt. The host +2. It carries each part of the ruling — the act (plan the round first), the unit (one response, several + readings), the boundary (a call whose arguments depend on another's answer waits), **when the plan may + be revised** (between rounds, never between two calls of one round), and the reason (cost, not speed). +3. Each of the two action phrases is asserted **on its own**, per reader, so a mutation that removes one + is caught by a case naming it rather than by a longer assertion that happens to still be true. +4. The three misstatements are **gone**: no `parallel`, no `for speed`, no 并发. One statement, not two. +5. **Reverse**: no wording that caps, budgets or quotas *rounds* may enter the shared prompt. The host rejected a round budget explicitly (rant 2026-10-15T12:35:03, 「c 没意义，肯定不行」 — it truncates long tasks), so a future edit must not smuggle one in beside the rule that saves rounds. @@ -76,15 +87,29 @@ #: The rule's identity: the bullet's lead phrase. Written as the phrase a rewrite would keep, so a second #: copy anywhere in the template is visible as a count of 2 rather than as longer prose. -RULE_LEAD = "**Issue every call you can already specify, in one response.**" +RULE_LEAD = "**Plan the round before the first call.**" + +#: The two actions the rule is made of, each a verbatim substring of the shipped wording and each asserted +#: on its own so that dropping one reddens a test naming it. **Enumerating** is the act the rule asks for +#: before any call; **writing the enumeration out** is the same act made observable, and it is the half a +#: wording can lose while still reading like advice. Measured on the previous bullet — which asked for a +#: count and carried neither phrase — the enumeration is what a reader never produced: 114,898 of 124,272 +#: lone rounds (92.5%) carried an `intent` naming no awaited answer, read by +#: `scripts/check-batching-share.py` over 137,592 rounds of this installation's history. +RULE_ACTION_PHRASES = ( + "Plan the round before the first call", + "write that enumeration out as the opening lines of your reply", +) #: One term per part of the ruling, each a verbatim substring of the shipped wording — so this list cannot #: drift away from the sentence it checks without this file saying so. RULE_TERMS = ( - RULE_LEAD, # the unit: one response, several calls - "a call goes alone only when its arguments need an answer you have not read yet", # the boundary - "names the specific answer it waits for", # what the lone call must still say - "Every round re-sends the conversation and emits fresh output", # the reason: money + RULE_LEAD, # the act: plan first, then call + "Every reading you can already specify goes out in one response", # the unit: one response, several + "a call stands alone only when its arguments need an answer you have not read yet", # the boundary + "The plan is revised between rounds, when a reading changes what the round can still settle", + "never between two calls of the same round", # when the plan may be revised — and not + "One round re-sends the whole conversation and emits fresh output", # the reason: money ) #: The precedence clause's carrier: the section that owns the red lines, whose own paragraph says no @@ -197,6 +222,29 @@ def test_the_shared_prompt_states_the_several_calls_rule(read) -> None: ) +@pytest.mark.parametrize("phrase", RULE_ACTION_PHRASES, ids=["plan-the-round", "write-the-enumeration"]) +@pytest.mark.parametrize("read", [ + pytest.param(lambda: SYSTEM_PROMPT.read_text(encoding="utf-8"), id="template"), + pytest.param(_render_system_prompt, id="render"), +]) +def test_each_action_phrase_carries_its_own_case(read, phrase) -> None: + """One case per action phrase, per reader — so deleting one reddens the test that names it. + + `test_the_shared_prompt_states_the_several_calls_rule` asserts the whole bullet at once, which is the + right shape for "is the rule stated" and the wrong one for "did this half survive": a rewrite that + dropped a single phrase would report the other terms as present and this file as passing. The two + phrases are what the rule asks a reader to *do* — enumerate what the round needs, and write that + enumeration where the host can see it — and each has to be able to fail on its own for a mutation arm + to mean anything. + """ + block = _how_you_work_block(read()) + assert block, f"system.j2 has no `{RULE_SECTION}` section for the rule to sit in" + assert phrase in block, ( + f"the rule's action `{phrase}` is missing from `{RULE_SECTION}`: the bullet asks a reader to act, " + "and an act it no longer names is an act no session performs" + ) + + def test_the_batching_rule_never_licenses_backgrounding() -> None: """The red line's precedence, in the shape the compacted prompt gives it.