fix(daemon): address post-merge review feedback on #1826

[igorls]

Follow-up to #1826 (merged), addressing the five points from the Copilot review. All are real, verified issues; #3 was also pre-existing on the plain mempalace sync CLI path.

Fixes

1. Privacy — WAL/SHM sidecar permissions. QueueStore created queue.sqlite3-wal/-shm (which hold un-checkpointed verbatim payloads) before run_server tightened the umask, and only the main .sqlite3 was chmod'd. The owner-only umask is now set before DaemonRuntime builds the queue, and _init_db hardens any existing sidecars as defense-in-depth.

2. DoS guard — negative Content-Length. Content-Length: -1 → rfile.read(-1) blocked until client disconnect and bypassed MAX_BODY_BYTES. Now rejected with a 400.

3. Import side effects — extract WAL. service.run_sync/cli.cmd_sync got _wal_log via from .mcp_server import _wal_log, which runs mcp_server's import-time stdio protection (os.dup2(2, 1), sys.stdout = sys.stderr) in a non-MCP process and misroutes operator output. _wal_log (+ _ensure_wal, _WAL_FILE, _WAL_REDACT_KEYS) now live in a new side-effect-free mempalace/wal.py; mcp_server/cli/service import from there.

4. Correctness — run_mcp_tool success inference. A write tool returning a bare {"error": ...} (e.g. tool_create_tunnel/tool_delete_tunnel validation) was recorded as succeeded. The error key now infers failure.

5. Hook budget — liveness-probe timeout. get_client_if_running()/health() take an explicit timeout; the hook "is the daemon up?" precheck uses a short HOOK_PROBE_TIMEOUT (0.5s) so a wedged daemon can't stall the hook for the default 5s.

Tests

New tests/test_wal.py (import isolation in a clean subprocess + redaction) and daemon tests for the umask ordering (POSIX-only), negative Content-Length (no hang), run_mcp_tool error inference, and the short probe timeout. Existing WAL tests in test_mcp_server.py/test_sync.py repointed to mempalace.wal.

Full suite green locally (2718 passed, 241 skipped); ruff check + format clean.

[gemini-code-assist]

Code Review

This pull request refactors the write-ahead log (WAL) implementation into a side-effect-free mempalace/wal module to prevent unwanted stdio redirection during CLI sync and daemon operations. It also hardens SQLite WAL/SHM sidecars, tightens the umask before database creation, rejects negative Content-Length headers to prevent DoS, and adds a short timeout for daemon liveness probes. The reviewer noted that if an exception occurs during server initialization, the tightened umask may not be restored, and suggested wrapping the setup in a try/except block to handle this.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If an exception is raised during initialization (for example, in ensure_token or DaemonRuntime due to file system or database errors), the try block starting at line 785 is never entered. As a result, the finally block at line 805 is skipped, and the process-global umask is never restored to its original value.

To prevent leaking this side effect on initialization failure, wrap the setup in a try/except block to restore the umask immediately if an error occurs.

Suggested change

	prev_umask = os.umask(0o077)
	token = ensure_token(palace_path)
	runtime = DaemonRuntime(palace_path, backend=backend)
	prev_umask = os.umask(0o077)
	try:
	token = ensure_token(palace_path)
	runtime = DaemonRuntime(palace_path, backend=backend)
	except BaseException:
	os.umask(prev_umask)
	raise

[Copilot]

Pull request overview

This PR is a follow-up hardening pass on the opt-in local daemon introduced in #1826, addressing privacy, correctness, and operational-budget issues discovered in post-merge review. It also refactors the write-ahead-log (WAL) helper into a side-effect-free module so non-MCP processes can audit writes without triggering MCP stdio redirection.

Changes:

• Harden daemon privacy/robustness: set owner-only umask before queue creation, chmod SQLite WAL/SHM sidecars, reject negative Content-Length, and use a short hook liveness probe timeout.
• Fix daemon job correctness by inferring run_mcp_tool failure from a bare {"error": ...} result.
• Extract WAL logging into mempalace/wal.py and update CLI/service/tests accordingly; add targeted tests for import isolation and redaction.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
mempalace/wal.py	New side-effect-free WAL module (redaction + lazy dir init) to avoid importing mcp_server in non-MCP contexts.
mempalace/mcp_server.py	Removes inline WAL implementation; re-exports _wal_log from mempalace.wal.
mempalace/service.py	Uses wal._wal_log for sync auditing; fixes run_mcp_tool success inference for {"error": ...} returns.
mempalace/cli.py	CLI sync path now imports _wal_log from mempalace.wal.
mempalace/daemon.py	Umask ordering fix, WAL/SHM chmod defense-in-depth, negative Content-Length guard, and explicit health probe timeouts (hook budget).
mempalace/hooks_cli.py	Hook daemon-availability probe now uses short HOOK_PROBE_TIMEOUT.
tests/test_wal.py	New tests for WAL import isolation (subprocess) and redaction/write smoke test.
tests/test_sync.py	Updates WAL monkeypatch target from mcp_server to wal.
tests/test_mcp_server.py	Updates WAL-related tests to use mempalace.wal.
tests/test_daemon.py	Adds regression tests for umask ordering, negative Content-Length, run_mcp_tool error inference, and short probe timeout.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.