From 5cc72cff23225b50d656b36e1bc0cfed3fbf1c0e Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Sun, 11 Oct 2026 09:07:41 -0400 Subject: [PATCH] fix(keyword): retain distinct downloaded document replies --- ...R-028-keyword-unique-document-downloads.md | 19 ++++++++ plugins/keyword/keyword.py | 8 +++- tests/test_keyword_attachment_urls.py | 3 +- .../test_keyword_unique_document_downloads.py | 46 +++++++++++++++++++ 4 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 docs/adr/ADR-028-keyword-unique-document-downloads.md create mode 100644 tests/test_keyword_unique_document_downloads.py diff --git a/docs/adr/ADR-028-keyword-unique-document-downloads.md b/docs/adr/ADR-028-keyword-unique-document-downloads.md new file mode 100644 index 00000000..e936037a --- /dev/null +++ b/docs/adr/ADR-028-keyword-unique-document-downloads.md @@ -0,0 +1,19 @@ +# ADR 028: fix(keyword): retain distinct downloaded document replies + +Status: Accepted for this candidate. + +## Context + +Every keyword document download is saved as tmp/. Two different URLs ending in report.pdf overwrite the same file before queued replies send it, so an earlier reply delivers the later document. + +## Decision + +Allocate a fresh managed temporary directory for each successful download, preserving the original basename used for outgoing display. + +## Consequences + +Sequential and concurrent replies keep their own bytes and filenames. The files remain inside the existing managed tmp area; failed downloads create no staging directory. + +## Validation + +The focused regression fails on the unmodified base and passes with this change. tests/test_keyword_unique_document_downloads.py tests/test_keyword_attachment_urls.py passed locally; changed Python files passed CI-equivalent Ruff F checks. These are local native-contract checks using controlled doubles, not live provider or channel acceptance. diff --git a/plugins/keyword/keyword.py b/plugins/keyword/keyword.py index b339b329..4412f1bb 100644 --- a/plugins/keyword/keyword.py +++ b/plugins/keyword/keyword.py @@ -2,6 +2,8 @@ import json import os +import tempfile +from pathlib import Path import requests from urllib.parse import urlparse import plugins @@ -72,7 +74,7 @@ class Keyword(Plugin): # 下载到 Agent 受管的 tmp 目录。不要自己拼 "tmp"：那是相对进程 CWD # 解析的，打包后的桌面端控制不了 CWD，甚至可能没有写权限—— # 原来那句 makedirs 会直接抛错，被外层 except 吞掉后关键词静默不回复。 - file_path = state_dir.tmp_dir() / file_name + tmp_dir = state_dir.tmp_dir() # Bound the request. The reply is built on the thread handling # this message, and requests with no timeout waits forever, so a # file host that accepts the connection and then stalls would @@ -102,6 +104,10 @@ class Keyword(Plugin): reply.type = ReplyType.ERROR reply.content = f"下载失败：{failure}" else: + # Replies may wait in the send queue. A fresh directory + # keeps earlier bytes intact without changing the display name. + download_dir = Path(tempfile.mkdtemp(prefix="keyword_", dir=str(tmp_dir))) + file_path = download_dir / file_name file_path.write_bytes(response.content) reply = Reply() reply.type = ReplyType.FILE diff --git a/tests/test_keyword_attachment_urls.py b/tests/test_keyword_attachment_urls.py index fb2579d8..5a0e2eb6 100644 --- a/tests/test_keyword_attachment_urls.py +++ b/tests/test_keyword_attachment_urls.py @@ -68,7 +68,8 @@ def test_xlsx_url_uses_path_filename_without_query_string(tmp_path): assert reply.type is ReplyType.FILE # The query string must not leak into the saved name. assert Path(reply.content).name == "report.XLSX" - assert Path(reply.content).parent == expected_dir + assert Path(reply.content).parent.parent == expected_dir + assert Path(reply.content).parent.name.startswith("keyword_") assert Path(reply.content).read_bytes() == b"spreadsheet" finally: set_agent_registry(None) diff --git a/tests/test_keyword_unique_document_downloads.py b/tests/test_keyword_unique_document_downloads.py new file mode 100644 index 00000000..a5c12c9d --- /dev/null +++ b/tests/test_keyword_unique_document_downloads.py @@ -0,0 +1,46 @@ +"""Pending keyword file replies must not overwrite one another's bytes.""" +import importlib +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from types import SimpleNamespace + +import plugins +from bridge.context import Context, ContextType +from bridge.reply import ReplyType +from plugins import Event, EventContext + +plugins.instance.current_plugin_path = "./plugins/keyword" +keyword_module = importlib.import_module("plugins.keyword.keyword") +plugins.instance.current_plugin_path = None +Keyword = plugins.instance.plugins["KEYWORD"] + + +def handle(url): + plugin = Keyword.__new__(Keyword) + plugin.keyword = {"download": url} + event = EventContext(Event.ON_HANDLE_CONTEXT, {"context": Context(ContextType.TEXT, "download"), "reply": None}) + plugin.on_handle_context(event) + return event["reply"] + + +def test_two_urls_with_same_basename_retain_each_document(monkeypatch, tmp_path): + monkeypatch.setattr(keyword_module.state_dir, "tmp_dir", lambda: tmp_path) + monkeypatch.setattr(keyword_module.requests, "get", lambda url, **kwargs: SimpleNamespace(status_code=200, content=url.encode())) + first = handle("https://one.invalid/report.pdf") + second = handle("https://two.invalid/report.pdf") + assert first.type is second.type is ReplyType.FILE + assert first.content != second.content + assert Path(first.content).read_bytes() == b"https://one.invalid/report.pdf" + assert Path(second.content).read_bytes() == b"https://two.invalid/report.pdf" + assert Path(first.content).name == Path(second.content).name == "report.pdf" + + +def test_simultaneous_downloads_keep_all_reply_paths(monkeypatch, tmp_path): + monkeypatch.setattr(keyword_module.state_dir, "tmp_dir", lambda: tmp_path) + monkeypatch.setattr(keyword_module.requests, "get", lambda url, **kwargs: SimpleNamespace(status_code=200, content=url.encode())) + urls = [f"https://host{i}.invalid/report.pdf" for i in range(8)] + with ThreadPoolExecutor(max_workers=8) as pool: + replies = list(pool.map(handle, urls)) + assert len({reply.content for reply in replies}) == len(urls) + for url, reply in zip(urls, replies): + assert Path(reply.content).read_bytes() == url.encode()