Skip to content

Commit 625a47f

Browse files
jpheinclaude
andcommitted
fix(/mine): address Copilot review on #1
Three findings from Copilot's review pass: 1. **Path-join normalization** (Copilot main.py:223). _translate_client_path built the rewrite via raw string concatenation; mismatched trailing slashes between client_prefix and daemon_prefix could produce paths like "/mnt/raid/ccprojects/-x". Strip exactly one trailing / from daemon_prefix, lstrip from suffix, join with explicit "/" so the result is a single separator regardless of operator slash style. 2. **Type validation in /mine** (Copilot main.py:1068). `directory` comes from JSON and could be a number / object / list, which would raise on _translate_client_path().startswith and surface as 500. Add isinstance(directory, str) guard returning a clean 400. 3. **Test determinism for _parse_path_map** (Copilot tests/test_path_translation.py:31). The previous test passed _parse_path_map(None) and assumed env was unset, but None used to mean "read env." Replace the None default with a private sentinel (_PATH_MAP_USE_ENV) so: - _parse_path_map() — read env (sentinel default) - _parse_path_map(None) — no mapping - _parse_path_map(str) — parse this Updated tests are split: one for explicit None == empty, one for no-arg + cleared env == empty, one for no-arg + set env == parsed. New tests: - test_explicit_none_returns_empty - test_env_unset_no_arg_returns_empty - test_join_normalizes_mismatched_trailing_slash (4 cases: each combo of trailing/no-trailing slash on client and daemon prefix) 13 tests pass: python -m unittest tests.test_path_translation -v Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6c8567f commit 625a47f

2 files changed

Lines changed: 78 additions & 6 deletions

File tree

main.py

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,19 +176,31 @@ def _check_auth(x_api_key: str | None):
176176
raise HTTPException(status_code=401, detail="Invalid API key")
177177

178178

179-
def _parse_path_map(raw: str | None = None) -> list[tuple[str, str]]:
179+
# Sentinel for "no value passed" — distinguishes _parse_path_map() (read env)
180+
# from _parse_path_map(None) (no mapping). Closes Copilot's test-isolation
181+
# concern on jphein/palace-daemon#1: the previous None default coupled tests
182+
# to whatever PALACE_DAEMON_PATH_MAP happened to be in the test process env.
183+
_PATH_MAP_USE_ENV: object = object()
184+
185+
186+
def _parse_path_map(raw=_PATH_MAP_USE_ENV) -> list[tuple[str, str]]:
180187
"""Parse PALACE_DAEMON_PATH_MAP into ordered (client_prefix, daemon_prefix) pairs.
181188
182189
Format: comma-separated ``client_prefix=daemon_prefix`` entries. Whitespace
183190
around each token is stripped. Empty entries and entries missing ``=`` are
184191
skipped silently. Order is preserved so the operator can put more-specific
185192
prefixes first.
186193
194+
Args:
195+
raw: When omitted, reads from ``PALACE_DAEMON_PATH_MAP``. Pass an
196+
explicit string (or ``""``/``None``) to bypass env entirely —
197+
tests use this to stay deterministic regardless of CI / dev env.
198+
187199
Example::
188200
189201
PALACE_DAEMON_PATH_MAP="/home/jp/.claude/=/mnt/raid/claude-config/,/home/jp/Projects/=/mnt/raid/projects/"
190202
"""
191-
if raw is None:
203+
if raw is _PATH_MAP_USE_ENV:
192204
raw = os.environ.get("PALACE_DAEMON_PATH_MAP", "")
193205
raw = (raw or "").strip()
194206
if not raw:
@@ -217,10 +229,15 @@ def _translate_client_path(path: str) -> str:
217229
218230
The first matching prefix wins; non-matching paths pass through
219231
unchanged so daemon-side absolute paths still work.
232+
233+
Joining is normalized so mismatched trailing/leading slashes between
234+
the two prefixes can't produce paths like ``/mnt/raid/ccprojects/...``
235+
(Copilot finding on jphein/palace-daemon#1).
220236
"""
221237
for client_prefix, daemon_prefix in _parse_path_map():
222238
if path.startswith(client_prefix):
223-
return daemon_prefix + path[len(client_prefix):]
239+
suffix = path[len(client_prefix):]
240+
return daemon_prefix.rstrip("/") + "/" + suffix.lstrip("/")
224241
return path
225242

226243

@@ -1061,6 +1078,11 @@ async def mine(request: Request, x_api_key: str | None = Header(default=None)):
10611078
directory = body.get("dir")
10621079
if not directory:
10631080
raise HTTPException(status_code=400, detail="'dir' is required")
1081+
if not isinstance(directory, str):
1082+
# Closes Copilot finding on jphein/palace-daemon#1: a JSON number /
1083+
# object / list would crash _translate_client_path().startswith and
1084+
# surface as 500 rather than a clean 400.
1085+
raise HTTPException(status_code=400, detail="'dir' must be a string")
10641086

10651087
# Hook clients send paths in their own filesystem namespace. Translate
10661088
# to the daemon's view via PALACE_DAEMON_PATH_MAP before validation.

tests/test_path_translation.py

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,16 @@
2626

2727

2828
class TestParsePathMap(unittest.TestCase):
29-
def test_empty_returns_empty(self):
29+
def test_empty_string_returns_empty(self):
3030
self.assertEqual(main._parse_path_map(""), [])
31-
self.assertEqual(main._parse_path_map(None), [])
3231
self.assertEqual(main._parse_path_map(" "), [])
3332

33+
def test_explicit_none_returns_empty(self):
34+
# None now means "no mapping" (does NOT fall back to env). The
35+
# sentinel default keeps the env-reading behavior on the no-arg
36+
# call below — closes Copilot finding on jphein/palace-daemon#1.
37+
self.assertEqual(main._parse_path_map(None), [])
38+
3439
def test_single_pair(self):
3540
out = main._parse_path_map("/home/u/.claude/=/mnt/raid/claude-config/")
3641
self.assertEqual(out, [("/home/u/.claude/", "/mnt/raid/claude-config/")])
@@ -57,12 +62,18 @@ def test_strips_whitespace_around_tokens(self):
5762
self.assertEqual(out, [("/a/", "/b/")])
5863

5964
def test_reads_env_var_when_no_arg(self):
65+
# No-arg call reads env. clear=True so a stray PALACE_DAEMON_PATH_MAP
66+
# in the test process can't taint the assertion.
6067
with patch.dict(
61-
os.environ, {"PALACE_DAEMON_PATH_MAP": "/x/=/y/"}, clear=False
68+
os.environ, {"PALACE_DAEMON_PATH_MAP": "/x/=/y/"}, clear=True
6269
):
6370
out = main._parse_path_map()
6471
self.assertEqual(out, [("/x/", "/y/")])
6572

73+
def test_env_unset_no_arg_returns_empty(self):
74+
with patch.dict(os.environ, {}, clear=True):
75+
self.assertEqual(main._parse_path_map(), [])
76+
6677

6778
class TestTranslateClientPath(unittest.TestCase):
6879
def test_passthrough_when_no_map(self):
@@ -106,6 +117,45 @@ def test_multiple_rules_first_match_wins(self):
106117
"/mnt/raid/projects/realmwatch/foo",
107118
)
108119

120+
def test_join_normalizes_mismatched_trailing_slash(self):
121+
"""Closes Copilot finding on jphein/palace-daemon#1: prefixes with
122+
mismatched trailing slashes used to produce paths like
123+
``/mnt/raid/ccprojects/...``. Now the join normalizes to exactly
124+
one separator regardless of operator slash style.
125+
"""
126+
# client trailing /, daemon no trailing
127+
with patch.dict(
128+
os.environ, {"PALACE_DAEMON_PATH_MAP": "/home/u/.claude/=/mnt/raid/cc"}, clear=True
129+
):
130+
self.assertEqual(
131+
main._translate_client_path("/home/u/.claude/projects/-x"),
132+
"/mnt/raid/cc/projects/-x",
133+
)
134+
# client no trailing, daemon trailing /
135+
with patch.dict(
136+
os.environ, {"PALACE_DAEMON_PATH_MAP": "/home/u/.claude=/mnt/raid/cc/"}, clear=True
137+
):
138+
self.assertEqual(
139+
main._translate_client_path("/home/u/.claude/projects/-x"),
140+
"/mnt/raid/cc/projects/-x",
141+
)
142+
# both trailing /
143+
with patch.dict(
144+
os.environ, {"PALACE_DAEMON_PATH_MAP": "/home/u/.claude/=/mnt/raid/cc/"}, clear=True
145+
):
146+
self.assertEqual(
147+
main._translate_client_path("/home/u/.claude/projects/-x"),
148+
"/mnt/raid/cc/projects/-x",
149+
)
150+
# neither trailing /
151+
with patch.dict(
152+
os.environ, {"PALACE_DAEMON_PATH_MAP": "/home/u/.claude=/mnt/raid/cc"}, clear=True
153+
):
154+
self.assertEqual(
155+
main._translate_client_path("/home/u/.claude/projects/-x"),
156+
"/mnt/raid/cc/projects/-x",
157+
)
158+
109159

110160
if __name__ == "__main__":
111161
unittest.main()

0 commit comments

Comments
 (0)