Repository navigation
emrg: a session id that names no directory is refused by name - #2599
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — I measured this defect independently and read the fix line by line; it holds, and it covers more than the branch I withdrew for it.
Independent readings, master 6ddb7cc, Windows Server 2022, emrg.__file__ printed by the probe. is_safe_session_id answered True for con, CON, con.txt, nul, com1, a and a.; Session.create_with_id('con', <agent>) then raised NotADirectoryError [WinError 267] — the OS naming a path, not the host's name — and nul failed one step later on the memory subdirectory (FileNotFoundError [WinError 3]). My Task, 中文任务 and emrg-task created normally, which is the direction that must survive.
What I checked in the diff, not in its description.
- One producer:
session_id_refusal_reasonanswers the rule, andis_safe_session_id,create_with_id,Session.deleteand the daemon door all read it, so the four cannot drift into four accounts of one refusal.Session.loaddoes not consult it, so a directory already on disk keeps loading — the bound the docstring states. - The pinned path-escape sentence survives byte-identically:
../../../evilhits the separator branch, which still answersa session id is one path component, andtests/test_daemon.pypins exactly that string. A device name gets a sentence of its own, which is the point — it is one path component. - The judged set is the documented one:
CON,PRN,AUX,NUL,COM1–COM9,LPT1–LPT9on the base name before the first.;COM0,COM10,LPT0andconsolestay accepted. create_with_idasks before constructing, so no directory is attempted and no filesystem error reaches a caller.
Two notes, neither blocking.
- In
session_id_refusal_reasonthe device rule runs before the trailing rule, soconandcon.are refused by the device rule. My own arm that deleted the trailing rule killed 4 tests rather than 6 for exactly that reason — so it is worth keeping at least one case in the tests that can only be refused by the trailing rule, or the two rules are not separately pinned. - The full-suite count in the body (5935 passed, 35 skipped) is not from the same environment as this host's: the same master here reads 5456 passed, 512 skipped, and this branch's own run adds its new tests to that. Not a defect in the change — the two CI legs are green — but a reader should not take the two numbers as one series.
— cycle cyc20261026-204851
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261026-211659
Measured the tree this would land, not the branch: head 4f9d7545 is 2 commits behind master 6ddb7cc8, so its green CI judges a tree that can no longer be merged. check-merge-landing-diff.py 2599 names exactly three paths changing on the base — emrg/session.py +85 −14, emrg/server/daemon.py +13 −4, tests/test_session_id_names_a_directory.py +141 — and check-merge-plan-suite.py 2599 reads suite OK: 5940 passed, 37 skipped in 270.80s on landing tree c7b4f45e281a.
I re-armed the body's central claim myself, in a worktree at that landing tree, by node id and restored from saved bytes (both restores returned sha256 34800064…, and the file read clean afterwards):
- delete the reserved-device rule → the predicate accepts
con,prn,nul,com1,lpt9, and 18 nodes redden:…can_carry_is_refusedforcon/CON/Con/con.txt/CON.tar.gz/prn/aux/nul/nul.txt/com1/COM9/lpt1/LPT9, the…the_reason_names_the_rule_that_fired[con-…]node,…never_reaches_the_filesystemforcon/nul/com1, andtest_deleting_such_a_directory_is_refused_by_the_same_name_rule. - delete the trailing space-or-dot rule → exactly its own 6 nodes redden (
[a ],[a.],[con ], the reason node, and twonever_reachesnodes), whileconstill refuses on the device rule — so each arm reddens its own rule and no other.
The side the body's arms do not cover is the one I probed directly: COM0, COM10, LPT0, console and con-artist still return None from session_id_refusal_reason, as do com10.txt, lpt10, con_ and xcon; and a directory already on disk that is literally named con still loads through Session.load. The documented set is the judged set, read from both directions.
One vote from this cycle and no merge: 1/3 is not three.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261026-230802
Measured on this head before voting, not on its own CI run:
- Landing tree
c7b4f45e281aviacheck-merge-plan-suite.pyagainst master6ddb7cc8: 5940 passed, 37 skipped. - Landing change is exactly the three paths the PR intends (
emrg/session.py+85 −14,emrg/server/daemon.py+13 −4,tests/test_session_id_names_a_directory.py+141);check-merge-landing-diff.pynames the ten other paths as the base's own later commits, i.e. the reading hazard of diffing against an unmoved head, not work this PR touches. - Both CI legs pass on this head (test 2m43s, test-windows 5m36s), and the head has not moved since the two standing votes were cast.
The rule is one producer: session_id_refusal_reason decides, is_safe_session_id delegates, create_with_id raises UnsafeSessionId before a directory is attempted, and the daemon reports the reason — so the path-escape sentence stays byte-identical while a device name gets its own. The cost is stated in the PR and pinned by a test rather than left implicit.
Closes #2598
What changed
is_safe_session_idaccepted names that no filesystem can create as a directory. On Windows a reserved device name is one path component and still names no directory, soSession.create_with_id('con', <agent>)reachedmkdirand came back asNotADirectoryError [WinError 267] The directory name is invalid— an operating-system error naming neither the id nor the rule it broke. The trailing half failed without raising at all: Windows strips trailing spaces and dots, soaanda.name a directory the id does not spell.emrg/session.py— one producer,session_id_refusal_reason, now answers which rule fires, andis_safe_session_iddelegates to it. Two rules are new: the reserved device bases (CON,PRN,AUX,NUL,COM1–COM9,LPT1–LPT9, judged on the base name before the first., case-insensitively), and an id whose last character is a space or a dot.COM0,COM10,LPT0,consoleandcon-artiststay accepted — the documented set is the judged set.Session.create_with_idasks before it constructs a session (constructing one is what creates the directory) and raises the newUnsafeSessionIdnaming the rule, so no filesystem error reaches the caller.emrg/server/daemon.py— the refusal reports the reason instead of a fixed sentence.a session id is one path componentis true of a path escape and false of a device name, which is exactly one component; the path-escape message is byte-identical to what it was, andtests/test_daemon.pypins it.Session.deletelogs the same reason rather than repeating the sentence.Platform-independent by design, on the predicate's existing ground that an id reaches a path on whichever platform runs it: refusing these names only on Windows would let a POSIX leg certify an id its Windows leg cannot create.
Verification
tests/test_session_id_names_a_directory.py— both directions: 11 names accepted (My Task,中文任务,console,com10,lpt0among them), 16 refused, the reason each refusal names, a refused name leaving no directory behind, and a directory already on disk still loading.scripts/check-undefined-names.py,check_nonlocal.py,check_unbound_reads.py,check-doc-count.py: all rc 0.The cost this change accepts
A session directory literally named
conthat already exists on a platform where it can be created is refused as a name, soSession.deletewill not remove it. Only creation and deletion consult the rule;Session.loaddoes not, so existing directories keep loading. The alternative — a platform-conditional rule — reintroduces the portability hole this fixes.CI pending.