Address the verification of PR 609: a read-only file is refused, a la…

…te load installs no watch A 0444 file of the user's own in a writable directory took the replace branch and was swapped out; it is refused Permission denied as g_file_replace refused it. A read or write landing after EditorPane.shutdown filled the buffer and watched the file again (a restored tab closed soon after its files reopened); shutdown sets _shut and the landings install nothing. A chunked request's frames and its text go out in one I/O job; a save of a clean buffer during a silent reload is a no-op. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_011sbC3wuGjnM1AysiE7RjJ8
ghackett · Oct 5, 2026 · Oct 5, 2026 · Oct 5, 2026 · Oct 5, 2026 · Oct 5, 2026
commit a9d2b2ceccdddc4ec35c84a9d94461c7f026ecda
diff --git a/.agents/collins-editor-panel/SKILL.md b/.agents/collins-editor-panel/SKILL.md
@@ -108,8 +108,9 @@ files.py` the service's:
   writes as `FileSaver` did (`service/files.write_file`): in place for a
   hard link, a read-only directory or another owner's file, else by a temp
   file beside it with the old mode and one `os.replace`, a new file under
-  the umask; the compare runs right before the write, and the reply's
-  mtime is the written descriptor's. A save whose encoding differs from
+  the umask; a read-only file of the user's own is refused Permission
+  denied, never swapped out; the compare runs right before the write, and
+  the reply's mtime is the written descriptor's. A save whose encoding differs from
   the read's (latin-1 that could not carry the text) is told in the banner.
 - `_watch_external_changes` installs `fs.watch kind: file` under a handle
   the client mints (`remotefiles.Watcher`), seeded with `opened.mtime`:
@@ -122,7 +123,9 @@ files.py` the service's:
   its watches. `_teardown_page` unwatches one file; `EditorPane.shutdown`
   every one (`TerminalTab.release_editor`, called where the window closes
   a tab for good and when the window itself is destroyed) — without it
-  every watch, and through it the pane, outlived its tab. The service's
+  every watch, and through it the pane, outlived its tab; `shutdown` sets
+  `_shut`, so a read or write that lands after it fills nothing and
+  installs no watch (a restored tab closed soon after its files reopened). The service's
   `Gio.FileMonitor` debounces 300 ms, stats on a thread and pushes
   `file-changed {handle, path, mtime, size, gone}` once per burst whose
   stat moved. `_check_external` judges it against `opened.mtime` and

diff --git a/collins/api/client.py b/collins/api/client.py
@@ -457,21 +457,27 @@ def _send_request(self, channel: _Channel, message: dict) -> None:
             frames, slim = protocol.split_request(message)
         except ValueError as exc:
             raise RequestRefused(protocol.ERROR_INVALID, f"{message.get('t')}: {exc}", {}) from None
-        if frames:
-            encoded = [GLib.Bytes.new(frame) for frame in frames]
+        if not frames:
+            self._send_text(channel, slim)
+            return
+        encoded = [GLib.Bytes.new(frame) for frame in frames]
+        text = protocol.encode(slim)
 
-            def do_send_frames():
-                ws = channel.ws
-                if ws is None or ws.get_state() != Soup.WebsocketState.OPEN:
-                    return
-                try:
-                    for frame in encoded:
-                        ws.send_message(Soup.WebsocketDataType.BINARY, frame)
-                except GLib.Error as exc:
-                    log.error("api client: send failed: %s", exc)
+        def do_send():
+            # One job for the frames and the request behind them: two
+            # threads' chunked requests on one connection must not
+            # interleave, or the server joins the wrong bytes.
+            ws = channel.ws
+            if ws is None or ws.get_state() != Soup.WebsocketState.OPEN:
+                return
+            try:
+                for frame in encoded:
+                    ws.send_message(Soup.WebsocketDataType.BINARY, frame)
+                ws.send_text(text)
+            except GLib.Error as exc:
+                log.error("api client: send failed: %s", exc)
 
-            self._post(do_send_frames)
-        self._send_text(channel, slim)
+        self._post(do_send)
 
     def _blocking(self, channel: _Channel, message: dict, timeout: float) -> dict:
         """Send on *channel* and wait for the reply (any thread but the I/O

diff --git a/collins/editor.py b/collins/editor.py
@@ -147,6 +147,7 @@ def __init__(self, root: str | Path) -> None:
         self._root = Path(root)
         self._open: dict[str, _OpenFile] = {}  # path str -> _OpenFile
         self._watched: dict[str, _OpenFile] = {}  # a watch's handle -> the file it watches
+        self._shut = False  # `shutdown` ran: a read or write landing later installs nothing
         self._pages: dict[str, Adw.TabPage] = {}
         self._page_key: dict[Adw.TabPage, str] = {}
         self._close_confirmed: set[Adw.TabPage] = set()  # discard-changes already agreed to
@@ -893,6 +894,8 @@ def _open_image_page(self, key: str, path: Path) -> None:
         self._select_page(page)
 
     def _on_loaded(self, opened: _OpenFile, load_id: int, kind: str, value) -> None:
+        if self._shut:
+            return  # the tab closed while the read was in flight: nothing to fill or watch
         if load_id != opened.load_id:
             return  # a newer load (a rename's) owns this buffer now
         opened.loading = False
@@ -1022,6 +1025,12 @@ def _do_save(self, opened: _OpenFile, on_done=None, expect: int | None = None) -
             if on_done is not None:
                 opened.save_again.append(on_done)
             return
+        if opened.reloading and not opened.buffer.get_modified():
+            # Ctrl+S in the middle of a silent reload: nothing of the user's
+            # to write, and the read in flight is about to move the mtime.
+            if on_done is not None:
+                on_done(True)
+            return
         start, end = opened.buffer.get_bounds()
         text = opened.buffer.get_text(start, end, True)
         if text and opened.buffer.get_implicit_trailing_newline():
@@ -1036,6 +1045,10 @@ def _do_save(self, opened: _OpenFile, on_done=None, expect: int | None = None) -
 
     def _on_saved(self, opened: _OpenFile, on_done, expect: int | None, kind: str, value) -> None:
         opened.saving = False
+        if self._shut:
+            if on_done is not None:
+                on_done(kind == "ok")
+            return
         waiters, opened.save_again = opened.save_again, None
         if kind != "ok":
             refusal: RequestRefused = value
@@ -1128,6 +1141,8 @@ def _watch_external_changes(self, opened: _OpenFile) -> None:
         Idempotent: a file that gets re-watched (a rename, a restarted load)
         must not leave the watch on its old path running."""
         self._unwatch(opened)
+        if self._shut:
+            return
         handle = remotefiles.watcher().watch(str(opened.path), self._on_file_changed, mtime=opened.mtime)
         opened.watch_handle = handle
         self._watched[handle] = opened
@@ -1150,7 +1165,9 @@ def shutdown(self) -> None:
         """The pane's tab is closing for good: every file's watch on the
         service is dropped (`TerminalTab.release_editor`). Without this
         the watches, and through their listener the pane and its buffers,
-        would live as long as the module's watcher."""
+        would live as long as the module's watcher. A read or write still
+        in flight lands on `_shut` and installs nothing."""
+        self._shut = True
         for opened in list(self._open.values()):
             self._unwatch(opened)
         self._watched.clear()

diff --git a/collins/service/files.py b/collins/service/files.py
@@ -53,6 +53,7 @@
 
 from __future__ import annotations
 
+import errno
 import logging
 import os
 import stat as stat_mod
@@ -328,7 +329,9 @@ def write_file(
     write, truncate, fsync), so every link sees the text and the owner and
     mode stay; any other existing file by a temporary file beside it,
     given the old mode, compared again right before the one `os.replace`
-    (the window between the compare and the write is the replace alone);
+    (the window between the compare and the write is the replace alone;
+    a file the user may not write, 0444 of their own, is refused
+    Permission denied as `g_file_replace` refused it, never swapped out);
     a file that is not there is created ``0o666`` under the umask, as
     GLib creates one. The reply's fields: the mtime and size read off the
     written descriptor (the temporary's, before the replace: the inode
@@ -365,6 +368,11 @@ def write_file(
         finally:
             os.close(fd)
         return {"mtime": mtime_us(written), "size": written.st_size, "encoding": encoding}
+    if not os.access(real, os.W_OK):
+        # A read-only file of the user's own (0444 in a writable directory):
+        # the replace could swap it out, but the saver this replaces refused
+        # with Permission denied, and so does this (a chmod is a decision).
+        raise PermissionError(errno.EACCES, os.strerror(errno.EACCES), real)
     fd, temp = tempfile.mkstemp(prefix=".collins-", suffix=".tmp", dir=directory)
     try:
         try:

diff --git a/scripts/check_editor_save.py b/scripts/check_editor_save.py
@@ -19,9 +19,9 @@
 until Overwrite; a deleted file is told and marked dirty; a change that
 waited for a reload is judged even when the reload fails; a save before
 the first read lands writes nothing; a change between the read and the
-watch reaches the buffer (the watch's seed); the pane's shutdown drops
-every watch; a closed tab's watch is gone. The pane never opens a file
-itself.
+watch reaches the buffer (the watch's seed); a closed tab's watch is
+gone; the pane's shutdown drops every watch, and a read landing after it
+installs none. The pane never opens a file itself.
 
 This is a script, not a pytest test, on purpose: tests/conftest.py blocks
 the GTK-stack namespaces for the whole suite so local runs reproduce CI.
@@ -370,23 +370,8 @@ def read_then_write(path, *args, **kwargs):
     finally:
         remotefiles.read = real_read
 
-    # -- the pane's shutdown drops every watch (a session tab closing) ---------------
-    handles = [o.watch_handle for o in pane._open.values() if o.watch_handle]
-    check("every open file holds a watch before the shutdown", len(handles) == len(pane._open) and handles)
-    pane.shutdown()
-    check(
-        "shutdown drops every watch on the service",
-        not any(watcher.watching(h) for h in handles) and not pane._watched,
-        [h for h in handles if watcher.watching(h)],
-    )
-    check("and the files remember none", all(o.watch_handle is None for o in pane._open.values()))
-    # The rest of the check needs the watches back.
-    for o in list(pane._open.values()):
-        pane._watch_external_changes(o)
-    other = pane._open[second]
-    handle = other.watch_handle
-
     # -- closing a tab drops its watch -----------------------------------------------
+    handle = other.watch_handle
     pane._close_confirmed.add(pane._pages[second])
     pane._tab_view.close_page(pane._pages[second])
     if not wait_for(lambda: second not in pane._open):
@@ -410,6 +395,39 @@ def read_then_write(path, *args, **kwargs):
         pane._banner.get_title(),
     )
 
+    # -- the pane's shutdown drops every watch (a session tab closing) ---------------
+    # And a read still in flight when the tab closes installs nothing when
+    # it lands: a restored tab closed soon after its files reopened.
+    sixth = os.path.join(root, "sixth.txt")
+    with open(sixth, "w") as fh:
+        fh.write("late\n")
+    real_read = remotefiles.read
+
+    def slow_read(path, *args, **kwargs):
+        if path == sixth:
+            time.sleep(0.5)
+        return real_read(path, *args, **kwargs)
+
+    remotefiles.read = slow_read
+    try:
+        pane.open_file(sixth)
+        late = pane._open[sixth]
+        handles = [o.watch_handle for o in pane._open.values() if o.watch_handle]
+        check("every filled file holds a watch before the shutdown", len(handles) == len(pane._open) - 1)
+        pane.shutdown()
+        check(
+            "shutdown drops every watch on the service",
+            not any(watcher.watching(h) for h in handles) and not pane._watched,
+            [h for h in handles if watcher.watching(h)],
+        )
+        check("and the files remember none", all(o.watch_handle is None for o in pane._open.values()))
+        settle(1.5)  # the slow read lands after the shutdown
+        check("a read landing after the shutdown fills nothing", late.loading and not late.filled)
+        check("and installs no watch", late.watch_handle is None and not pane._watched, pane._watched)
+        check("and nothing of the pane's is watched", not any(watcher.watching(h) for h in handles))
+    finally:
+        remotefiles.read = real_read
+
     print(f"\n{PASSED} passed, {FAILED} failed")
     return 1 if FAILED else 0
 

diff --git a/tests/test_files.py b/tests/test_files.py
@@ -462,3 +462,24 @@ def test_read_of_a_fifo_is_refused_not_waited_on(served):
     with pytest.raises(inproc.RequestRefused) as refused:
         client.request({"t": "fs.read", "path": str(fifo)})
     assert refused.value.error == protocol.ERROR_REFUSED and "not a file" in refused.value.msgid
+
+
+def test_a_read_only_file_of_ones_own_is_refused_not_swapped_out(served):
+    """0444 in a writable directory: the replace branch could swap the file
+    out, but the saver this replaces refused with Permission denied, and
+    so does the service — the content and the mode are untouched."""
+    core, client, project, _events = served
+    if os.geteuid() == 0:
+        pytest.skip("root writes anywhere")
+    path = project / "ro.txt"
+    path.write_text("keep\n")
+    path.chmod(0o444)
+    try:
+        with pytest.raises(inproc.RequestRefused) as refused:
+            client.request({"t": "fs.write", "path": str(path), "text": "new\n", "expect_mtime": None})
+        assert refused.value.error == protocol.ERROR_FAILED
+        assert "Couldn't save" in refused.value.msgid and "denied" in refused.value.details["error"]
+        assert path.read_text() == "keep\n" and stat.S_IMODE(path.stat().st_mode) == 0o444
+        assert not [name for name in os.listdir(project) if name.startswith(".collins-")]
+    finally:
+        path.chmod(0o644)