feat(files): delete /files/content endpoint + extract _apply_optional_rename
Step 12/12 of docs/superpowers/plans/2026-05-17-files-overlay-rewrite.md.
End of Phase C — end of the rewrite plan.
Two cleanups in one commit:
1. Delete GET /overlays/<id>/files/content.
The legacy openEditorForFile in files-overlay.js was its only caller,
and Step 9 deleted that code path. grep confirms no remaining live
callers (the matches in .claude/worktrees/* are other in-flight
branches; matches in docs/ are plan/spec text describing the route's
history). Removed:
* The @bp.get route function (was already a thin wrapper around
_load_file_for_editing from Step 11)
* The endpoint's mention in the module docstring
* test_content_returns_text
* test_content_returns_415_for_binary
* test_content_404_for_non_files_overlay
* The /files/content entry in the batched "non-files-overlay 404s
everywhere" test
The _load_file_for_editing helper from Step 11 becomes single-caller
(only the edit route uses it now). Kept because the function name
gives the prelude a useful named concept and inlining would add ~17
lines of low-density logic into overlay_file_edit_page.
2. Extract _apply_optional_rename.
overlay_file_save and overlay_file_replace had near-identical rename
branches: safe_resolve_for_move → 422-on-traversal, 409-if-dst-exists,
mkdir-parents, os.rename → echo_path = new_path. Extracted into
_apply_optional_rename(overlay, path, new_path) → (write_target,
echo_path) | Response.
The helper handles both cases:
* Rename: atomic rename, returns (dst, new_path)
* No rename: safe_resolve_for_write, mkdir parents, returns
(write_target, path)
Save's "destination is not a file" 409 (creation branch) stays inline
in overlay_file_save — it's save-specific behavior that doesn't apply
to /replace (which assumes a file exists or creates one).
Subtle behavior change in /save: the prior code called
safe_resolve_for_write(new_path or path) upfront and then potentially
overrode write_target via safe_resolve_for_move. The new code only
calls one validator per branch. Confirmed equivalent: per
overlay_files.py:36-58 (safe_resolve_for_write) vs. lines 76-106
(safe_resolve_for_move), the dst-side checks are identical (root
escape, symlink refuse, parent-is-dir) and safe_resolve_for_move adds
strictly more (src must exist, cycle check for directory moves). pytest
covers the save/replace paths and stays green.
pytest: 580 → 577 passed, 1 skipped, 3 deselected. The -3 is the 3
deleted /files/content tests.
files_routes.py: ended at 735 lines. The plan estimated ~450 — the
delta is the new /files/new route (~43 lines, Step 5), the binary
template branch (Step 7), the _load_file_for_editing helper (Step 11),
and module-header expansions. The structural goal (no dead routes,
shared helpers across paths) is met.
End-of-plan summary:
* 1091-line files-overlay.js → 4 focused modules totaling 1191 lines
(core.js 247, editor.js 309, dialogs.js 212, uploads.js 423)
* Editor flows (text edit, binary replace, create new) all run
through URL-addressable modals (?modal= deep-linkable)
* Legacy <dialog id="files-editor-modal"> deleted
* /files/content deleted (dead endpoint)
* Shared helpers _load_file_for_editing + _apply_optional_rename
* pytest stayed green at every step
* Chromium-verified every step
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
mwiegand
parent
3facc323b6
commit
5f82950d7c
2 changed files with 48 additions and 105 deletions
|
|
@ -10,8 +10,6 @@ Read-only endpoints (any overlay):
|
|||
is refused.
|
||||
|
||||
Mutating endpoints (only `overlay.type == 'files'`, owner or admin):
|
||||
- `GET /overlays/<id>/files/content?path=` — JSON `{path, content}` for
|
||||
an editable text file, 415 if not editable.
|
||||
- `POST /overlays/<id>/files/save` — JSON `{path, content, new_path?}`,
|
||||
text-mode write with optional atomic rename.
|
||||
- `POST /overlays/<id>/files/replace` — multipart `path`, `file`,
|
||||
|
|
@ -243,24 +241,6 @@ def _load_file_for_editing(overlay_id: int, sub_path: str, user):
|
|||
return overlay, target, content, False, len(content.encode("utf-8"))
|
||||
|
||||
|
||||
@bp.get("/overlays/<int:overlay_id>/files/content")
|
||||
@require_login
|
||||
def overlay_file_content(overlay_id: int):
|
||||
"""Return `{path, content}` for an editable text file."""
|
||||
user = current_user()
|
||||
assert user is not None
|
||||
sub_path = request.args.get("path", "")
|
||||
|
||||
result = _load_file_for_editing(overlay_id, sub_path, user)
|
||||
if isinstance(result, Response):
|
||||
return result
|
||||
_overlay, _target, content, is_binary, _byte_count = result
|
||||
|
||||
if is_binary:
|
||||
return Response("not editable", status=415)
|
||||
return jsonify({"path": sub_path, "content": content})
|
||||
|
||||
|
||||
@bp.get("/overlays/<int:overlay_id>/files/edit")
|
||||
@require_login
|
||||
def overlay_file_edit_page(overlay_id: int):
|
||||
|
|
@ -355,6 +335,40 @@ def _validate_save_content(content: str) -> Response | None:
|
|||
return None
|
||||
|
||||
|
||||
def _apply_optional_rename(overlay, path: str, new_path: str | None):
|
||||
"""Resolve the write target for save / replace, atomically renaming
|
||||
when `new_path` differs from `path`. Returns:
|
||||
* (write_target, echo_path) on success — write_target is the
|
||||
resolved Path the caller should write to; echo_path is the
|
||||
canonical relative path to echo back in the response
|
||||
* a Flask Response on failure: 422 (path validation) or 409
|
||||
(rename destination already exists)
|
||||
|
||||
The atomic rename happens INSIDE this helper. Save's "destination
|
||||
is not a file" 409 (creation branch) stays in the caller, since
|
||||
/replace doesn't enforce that check."""
|
||||
if new_path is not None and new_path != path:
|
||||
try:
|
||||
src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
if dst_path.exists():
|
||||
return Response("destination already exists", status=409)
|
||||
dst_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
os.rename(src_path, dst_path)
|
||||
return dst_path, new_path
|
||||
try:
|
||||
write_target = safe_resolve_for_write(overlay.path, path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
write_target.parent.mkdir(parents=True, exist_ok=True)
|
||||
return write_target, path
|
||||
|
||||
|
||||
def _is_existing_file(path) -> bool:
|
||||
return path.is_file() and not path.is_symlink()
|
||||
|
||||
|
||||
@bp.post("/overlays/<int:overlay_id>/files/save")
|
||||
@require_login
|
||||
def overlay_file_save(overlay_id: int):
|
||||
|
|
@ -380,26 +394,15 @@ def overlay_file_save(overlay_id: int):
|
|||
return result
|
||||
overlay = result
|
||||
|
||||
try:
|
||||
write_target = safe_resolve_for_write(overlay.path, new_path or path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
rename_result = _apply_optional_rename(overlay, path, new_path)
|
||||
if isinstance(rename_result, Response):
|
||||
return rename_result
|
||||
write_target, echo_path = rename_result
|
||||
|
||||
# Rename branch: source must exist, dst must not collide.
|
||||
if new_path is not None and new_path != path:
|
||||
try:
|
||||
src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
if dst_path.exists():
|
||||
return Response("destination already exists", status=409)
|
||||
dst_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
os.rename(src_path, dst_path)
|
||||
write_target = dst_path
|
||||
else:
|
||||
write_target.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Creation branch: must not collide with an existing path.
|
||||
if write_target.exists() and not _is_existing_file(write_target):
|
||||
# Creation branch (no rename): must not collide with an existing
|
||||
# non-file (e.g. a directory at the same path).
|
||||
if (new_path is None or new_path == path) \
|
||||
and write_target.exists() and not _is_existing_file(write_target):
|
||||
return Response("destination is not a file", status=409)
|
||||
|
||||
try:
|
||||
|
|
@ -407,11 +410,7 @@ def overlay_file_save(overlay_id: int):
|
|||
except OSError as exc:
|
||||
return Response(f"write failed: {exc}", status=500)
|
||||
|
||||
return jsonify({"path": new_path or path})
|
||||
|
||||
|
||||
def _is_existing_file(path) -> bool:
|
||||
return path.is_file() and not path.is_symlink()
|
||||
return jsonify({"path": echo_path})
|
||||
|
||||
|
||||
@bp.post("/overlays/<int:overlay_id>/files/replace")
|
||||
|
|
@ -433,24 +432,10 @@ def overlay_file_replace(overlay_id: int):
|
|||
return result
|
||||
overlay = result
|
||||
|
||||
if new_path and new_path != path:
|
||||
try:
|
||||
src_path, dst_path = safe_resolve_for_move(overlay.path, path, new_path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
if dst_path.exists():
|
||||
return Response("destination already exists", status=409)
|
||||
dst_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
os.rename(src_path, dst_path)
|
||||
write_target = dst_path
|
||||
echo_path = new_path
|
||||
else:
|
||||
try:
|
||||
write_target = safe_resolve_for_write(overlay.path, path)
|
||||
except ValueError as exc:
|
||||
return Response(str(exc), status=422)
|
||||
write_target.parent.mkdir(parents=True, exist_ok=True)
|
||||
echo_path = path
|
||||
rename_result = _apply_optional_rename(overlay, path, new_path)
|
||||
if isinstance(rename_result, Response):
|
||||
return rename_result
|
||||
write_target, echo_path = rename_result
|
||||
|
||||
return _stream_upload_into(upload, write_target, echo_path)
|
||||
|
||||
|
|
|
|||
|
|
@ -582,47 +582,6 @@ def _csrf_headers():
|
|||
return {"X-CSRF-Token": "test-token"}
|
||||
|
||||
|
||||
# ---- /content -------------------------------------------------------------
|
||||
|
||||
|
||||
def test_content_returns_text(app, left4me_root: Path) -> None:
|
||||
user_id = _make_user()
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="files")
|
||||
overlay_dir = left4me_root / "overlays" / str(overlay_id)
|
||||
(overlay_dir / "motd.txt").write_text("welcome")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
r = client.get(f"/overlays/{overlay_id}/files/content?path=motd.txt")
|
||||
|
||||
assert r.status_code == 200
|
||||
assert r.get_json() == {"path": "motd.txt", "content": "welcome"}
|
||||
|
||||
|
||||
def test_content_returns_415_for_binary(app, left4me_root: Path) -> None:
|
||||
user_id = _make_user()
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="files")
|
||||
overlay_dir = left4me_root / "overlays" / str(overlay_id)
|
||||
(overlay_dir / "image.bin").write_bytes(b"\x00\x01\x02")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
r = client.get(f"/overlays/{overlay_id}/files/content?path=image.bin")
|
||||
|
||||
assert r.status_code == 415
|
||||
|
||||
|
||||
def test_content_404_for_non_files_overlay(app, left4me_root: Path) -> None:
|
||||
"""`content` is gated to type=='files' to keep the new editor scoped."""
|
||||
user_id = _make_user()
|
||||
overlay_id = _make_overlay(left4me_root, user_id=user_id, name="ws") # type='script' helper
|
||||
overlay_dir = left4me_root / "overlays" / str(overlay_id)
|
||||
(overlay_dir / "motd.txt").write_text("hi")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
r = client.get(f"/overlays/{overlay_id}/files/content?path=motd.txt")
|
||||
|
||||
assert r.status_code == 404
|
||||
|
||||
|
||||
# ---- /save ---------------------------------------------------------------
|
||||
|
||||
|
||||
|
|
@ -1064,7 +1023,6 @@ def test_mutating_endpoints_404_for_workshop_overlay(app, left4me_root: Path) ->
|
|||
("post", f"/overlays/{overlay_id}/files/delete",
|
||||
{"data": {"path": "x", "csrf_token": "test-token"}}),
|
||||
("get", f"/overlays/{overlay_id}/files/download_zip?path=", {}),
|
||||
("get", f"/overlays/{overlay_id}/files/content?path=x.txt", {}),
|
||||
]
|
||||
for method, url, kwargs in paths:
|
||||
kwargs = dict(kwargs)
|
||||
|
|
|
|||
Loading…
Reference in a new issue