fix(upload): archive extraction respects HERMES_WEBUI_ATTACHMENT_DIR

[OneFat3]

Thinking Path

• Hermes WebUI supports HERMES_WEBUI_ATTACHMENT_DIR to redirect uploads away from the workspace
• handle_upload() already honours this via _attachment_root()
• handle_upload_extract() ignores the override and hardcodes extraction into s.workspace
• This means archive uploads bypass the configured attachment directory
• This PR fixes handle_upload_extract() to use _attachment_root() consistently

What Changed

api/upload.py — handle_upload_extract() now calls _attachment_root() instead of using Path(s.workspace) as the extraction target.

Why It Matters

Users who set HERMES_WEBUI_ATTACHMENT_DIR (e.g. /workspace/uploads) expect all uploads — including archives — to land there. Without this fix, archives silently extract into the workspace root.

Verification

• Set HERMES_WEBUI_ATTACHMENT_DIR=/workspace/uploads
• Uploaded a zip archive via the UI
• Confirmed extraction landed in /workspace/uploads/<archive_stem>/ instead of /workspace/<archive_stem>/
• Single-file uploads continue to work as before

Risks / Follow-ups

Minimal — the change is two lines and only affects the archive extraction path. extract_archive() itself is unchanged; only the root directory passed to it differs.

Model Used

AI-assisted change with repository inspection, targeted editing, and shell-based test verification.

Ref #2247

[nesquena-hermes]

Summary

Reading api/upload.py:96-126 (handle_upload), :130-240 (extract_archive), and :242-269 (handle_upload_extract) on origin/master, plus the diff at :259-262 on this branch, the inconsistency the PR identifies is real:

• handle_upload() resolves uploads under _session_attachment_dir(session_id), which itself walks under _attachment_root() — honoring HERMES_WEBUI_ATTACHMENT_DIR per the helper's docstring at api/upload.py:65-76.
• handle_upload_extract() instead hardcoded Path(s.workspace) as the extraction root, which is the active workspace — bypassing the override and putting archive contents into the user's project tree.

The PR correctly redirects extract_archive's root to _attachment_root(). That restores parity between single-file uploads and archive uploads.

However, there are two concrete concerns worth raising before merge.

Code reference

The diff applied at api/upload.py:259-262:

-        workspace = Path(s.workspace)
-        result = extract_archive(file_bytes, filename, workspace)
+        upload_root = _attachment_root()
+        upload_root.mkdir(parents=True, exist_ok=True)
+        result = extract_archive(file_bytes, filename, upload_root)

Compare against the analogous single-file path at :114-115:

safe_name = _sanitize_upload_name(filename)
dest = _upload_destination(session_id, safe_name)  # → _session_attachment_dir(session_id)

Diagnosis / Recommendation

1. The fix loses session scoping. Single-file uploads go to _attachment_root() / <session_id_sanitized> via _session_attachment_dir() at api/upload.py:87-92. The new path for archives goes directly into _attachment_root() (the inbox root), not into a per-session subfolder. That means two sessions that upload archives with the same stem will collide on disk under the configured attachment dir — extract_archive() does append _NNN suffixes when the dir already exists (api/upload.py:151-156), so the second upload doesn't overwrite, but you also lose the "this archive came from session X" trail that single-file uploads keep. Recommendation: use _session_attachment_dir(session_id) as the root instead of _attachment_root() directly, e.g.:

upload_root = _session_attachment_dir(session_id)
upload_root.mkdir(parents=True, exist_ok=True)
result = extract_archive(file_bytes, filename, upload_root)

This keeps the session-scoping contract that handle_upload() already enforces and matches the user's mental model that "uploads from this chat go in this chat's folder."

2. extract_archive()'s relative_to(workspace.resolve()) at :194 and :228 will compute paths relative to the new root. The returned files list in the response now lists paths relative to _attachment_root() instead of s.workspace. Any frontend code that resolves those paths against the workspace will see them as orphans. Worth a grep for callers of /api/upload-extract and result.files to confirm nothing assumes a workspace-relative format:

grep -rn "extract_archive\|/api/upload-extract" ~/hermes-webui-public/{static,api}

If the WebUI just displays the relative path as text, the change is harmless; if it tries to open the file via the workspace file API, the path becomes broken. Worth confirming in the verification section.

3. Missing test coverage. The PR description says verification was a manual upload check, no automated test. The single-file upload path has tests under tests/test_upload*.py; the archive path should get a parallel test that:

• Sets HERMES_WEBUI_ATTACHMENT_DIR=/tmp/test-uploads
• POSTs an archive to /api/upload-extract
• Asserts the resulting files are under /tmp/test-uploads/<...> and not under s.workspace

That pins the behavior the PR claims to fix and prevents regression if someone later restores Path(s.workspace) in a refactor.

4. CHANGELOG entry is missing. This is a user-visible behavior change for anyone using HERMES_WEBUI_ATTACHMENT_DIR — archive uploads previously landed in the workspace, now they land in the inbox. Worth a release-note entry per the AGENTS.md "user-visible behavior" rule. Could also reference #2247 in the entry since this is the same feature surface.

Verification

The two-line minimal patch is correct in intent. The concerns above are about completeness (session subfolder, returned paths) and contract durability (test + changelog), not about whether the fix points at the right helper. Ref #2247 is the right framing — this slots into the configurable-attachment-directory work tracked in that issue.

[OneFat3]

Thanks for the thorough review! Addressed all four concerns in the updated commit:

1. Session scoping — Switched from _attachment_root() to _session_attachment_dir(session_id). Archives now land at <attachment_root>/<session_id>/<archive_stem>/, matching the single-file upload path and ensuring shutil.rmtree(_session_attachment_dir(sid)) covers them on session deletion.

2. Relative paths — Confirmed: the frontend (ui.js:7915) only consumes data.dest (absolute) and data.extracted (count). data.files is unused by the frontend. With workspace = session_dir, data.files now contains session-relative paths (e.g. archive_stem/file.txt), which is correct.

3. Tests — Added tests/test_pr2520_extract_attachment_dir.py with 4 test cases:

• Extraction lands in session dir (not bare root)
• Session cleanup (shutil.rmtree) covers extracted archives
• dest is scoped under session dir, not directly under inbox
• data.files paths are relative to session dir and resolve correctly

4. CHANGELOG — Added entry under [Unreleased] > Fixed.

All squashed into a single commit.