diff --git a/loopx/extensions/lark/return_files.py b/loopx/extensions/lark/return_files.py index bf66b0714c..e503900a57 100644 --- a/loopx/extensions/lark/return_files.py +++ b/loopx/extensions/lark/return_files.py @@ -19,6 +19,13 @@ from ...file_lock import exclusive_file_lock +# An upload acknowledgement is the resource prefix plus 1-240 token characters. +# The prefix is a property of the resource kind, not of the caller, so both +# shapes are stated once here instead of being rebuilt at each use site. +RESULT_FILE_KEY_PATTERN = re.compile(r"file_[A-Za-z0-9_-]{1,240}") +CHAT_IMAGE_KEY_PATTERN = re.compile(r"img_[A-Za-z0-9_-]{1,240}") + + def _call(args, cwd, *, runner, private_transport): try: if private_transport is not None: @@ -36,7 +43,8 @@ def uploaded_result_files(*, root, profile, provider_ref, attachments, runner=No never uploads replacement resources to reconstruct a sent-message intent. """ return _uploaded_resources(root=root, profile=profile, provider_ref=provider_ref, attachments=attachments, - cache_dir="lark-result-files", resource="files", key_field="file_key", key_prefix="file_", + cache_dir="lark-result-files", resource="files", key_field="file_key", + key_pattern=RESULT_FILE_KEY_PATTERN, read_bytes=lambda attachment: read_result_file(root, attachment), upload_data=lambda attachment: {"file_type": "stream", "file_name": attachment["name"]}, runner=runner, private_transport=private_transport, before_upload=before_upload, verify_only=verify_only) @@ -55,14 +63,15 @@ def uploaded_chat_images(*, root, profile, provider_ref, attachments, private_tr """Reuse resource IO; never regenerate an unknown upload or sent intent.""" return _uploaded_resources(root=root, profile=profile, provider_ref=provider_ref, attachments=_chat_images(attachments), cache_dir="lark-chat-images", resource="images", - key_field="image_key", key_prefix="img_", read_bytes=lambda image: image["raw"], + key_field="image_key", key_pattern=CHAT_IMAGE_KEY_PATTERN, + read_bytes=lambda image: image["raw"], upload_data=lambda _image: {"image_type": "message"}, runner=None, private_transport=private_transport, before_upload=before_upload, verify_only=verify_only, fence_upload=True) def _uploaded_resources(*, root, profile, provider_ref, attachments, cache_dir, resource, - key_field, key_prefix, read_bytes, upload_data, runner, private_transport, + key_field, key_pattern, read_bytes, upload_data, runner, private_transport, before_upload, verify_only, fence_upload=False): keys = [] for attachment in attachments: @@ -72,7 +81,7 @@ def _uploaded_resources(*, root, profile, provider_ref, attachments, cache_dir, with exclusive_file_lock(cache): saved = _read(cache) if cache.exists() else None if saved is not None: - if saved.get("identity") != identity or not re.fullmatch(key_prefix + r"[A-Za-z0-9_-]{1,240}", str(saved.get(key_field, ""))): + if saved.get("identity") != identity or not key_pattern.fullmatch(str(saved.get(key_field, ""))): raise ValueError("resource upload identity conflict or acknowledgement unavailable") keys.append(saved[key_field]) continue @@ -97,7 +106,7 @@ def _uploaded_resources(*, root, profile, provider_ref, attachments, cache_dir, uploaded = _call(args, cwd, runner=runner, private_transport=private_transport) payload = _json_object(uploaded.get("stdout")) key = (payload.get("data") or {}).get(key_field) - if uploaded.get("returncode") != 0 or payload.get("ok") is False or not re.fullmatch(key_prefix + r"[A-Za-z0-9_-]{1,240}", str(key or "")): + if uploaded.get("returncode") != 0 or payload.get("ok") is False or not key_pattern.fullmatch(str(key or "")): raise ValueError("result file upload unavailable") _write(cache, {"identity": identity, key_field: key}) keys.append(key) diff --git a/tests/extensions/test_lark_upload_key_shape.py b/tests/extensions/test_lark_upload_key_shape.py new file mode 100644 index 0000000000..133e1a67f4 --- /dev/null +++ b/tests/extensions/test_lark_upload_key_shape.py @@ -0,0 +1,150 @@ +"""The upload acknowledgement shape is one decision, taken the same way twice. + +A Lark upload returns `<1-240 characters of [A-Za-z0-9_-]>`, +and the prefix belongs to the resource kind (`file_` for result files, `img_` +for chat images), not to the caller. Both use sites -- reading a saved +acknowledgement back, and accepting a fresh one -- must answer from the same +pattern, so the boundary cases are pinned here rather than inherited from +whichever value a full delivery fixture happens to return. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from loopx.capabilities.manager_context import _root, _read, _write +from loopx.extensions.lark import return_files + +CACHE_DIR = "lark-result-files" + + +class _Runner: + """Answers the preflight and the create call, and records what was uploaded.""" + + def __init__(self, key: str | None) -> None: + self.key = key + self.creates = 0 + + def __call__(self, args: list[str]) -> dict: + if "--dry-run" in args: + return {"returncode": 0, "stdout": "{}"} + self.creates += 1 + if self.key is None: + return {"returncode": 0, "stdout": json.dumps({"ok": True, "data": {}})} + return { + "returncode": 0, + "stdout": json.dumps({"ok": True, "data": {"file_key": self.key}}), + } + + +def _upload(root: Path, *, key: str | None, pattern=return_files.RESULT_FILE_KEY_PATTERN, + runner: _Runner | None = None) -> tuple[str, _Runner]: + active = runner or _Runner(key) + keys = return_files._uploaded_resources( + root=root, + profile="steward-app", + provider_ref="om_provider", + attachments=[{"sha256": "a" * 64, "name": "result.bin", "size": 4}], + cache_dir=CACHE_DIR, + resource="files", + key_field="file_key", + key_pattern=pattern, + read_bytes=lambda _attachment: b"body", + upload_data=lambda _attachment: {"file_type": "stream", "file_name": "result.bin"}, + runner=active, + private_transport=None, + before_upload=lambda: True, + verify_only=False, + ) + return keys, active + + +def _saved_identity(root: Path) -> tuple[Path, dict]: + identity = { + "profile": "steward-app", + "provider_ref": "om_provider", + "sha256": "a" * 64, + "name": "result.bin", + "size": 4, + } + from loopx.control_plane.collaboration.inbox import _hash + + cache = _root(root) / CACHE_DIR / (_hash(identity) + ".json") + return cache, identity + + +@pytest.mark.parametrize("key", ["file_a", "file_" + "a" * 240, "file_A1-z_9"]) +def test_a_acknowledgement_within_the_shape_is_accepted(tmp_path: Path, key: str) -> None: + keys, runner = _upload(tmp_path, key=key) + + assert keys == (key,) + assert runner.creates == 1 + + +@pytest.mark.parametrize( + "key", + [ + "file_", # the tail is required, an empty body is not a key + "file_" + "a" * 241, # one character past the documented ceiling + "file_a.b", # '.' and ':' are event-id characters, not key ones + "file_a b", + "img_a", # the image prefix cannot answer a file upload + "filea", # the prefix is part of the contract + "", + ], +) +def test_an_acknowledgement_outside_the_shape_is_refused(tmp_path: Path, key: str) -> None: + with pytest.raises(ValueError, match="result file upload unavailable"): + _upload(tmp_path, key=key) + + +def test_the_image_prefix_accepts_what_the_file_prefix_refuses() -> None: + assert return_files.CHAT_IMAGE_KEY_PATTERN.fullmatch("img_a1-Z_9") + assert return_files.RESULT_FILE_KEY_PATTERN.fullmatch("file_a1-Z_9") + assert not return_files.CHAT_IMAGE_KEY_PATTERN.fullmatch("file_a1-Z_9") + assert not return_files.RESULT_FILE_KEY_PATTERN.fullmatch("img_a1-Z_9") + + +def test_a_saved_acknowledgement_outside_the_shape_is_not_reused( + tmp_path: Path, +) -> None: + """The read-back branch asks the same question as the upload branch.""" + + _upload(tmp_path, key="file_stored") + cache, identity = _saved_identity(tmp_path) + _write(cache, {"identity": identity, "file_key": "file_stored!!"}) + + with pytest.raises( + ValueError, match="identity conflict or acknowledgement unavailable" + ): + _upload(tmp_path, key="file_never_used") + + +def test_the_folded_patterns_are_the_construction_they_replace() -> None: + """Folding must not quietly re-decide the shape. + + Each pattern is asserted against the exact string the previous inline + concatenation built, so a changed character class or bound is a review + event rather than a silent widening of what an acknowledgement may contain. + """ + + tail = r"[A-Za-z0-9_-]{1,240}" + assert return_files.RESULT_FILE_KEY_PATTERN.pattern == f"file_{tail}" + assert return_files.CHAT_IMAGE_KEY_PATTERN.pattern == f"img_{tail}" + + +def test_a_saved_acknowledgement_in_shape_is_reused_without_uploading( + tmp_path: Path, +) -> None: + keys, _ = _upload(tmp_path, key="file_stored") + cache, _identity = _saved_identity(tmp_path) + runner = _Runner("file_second_upload") + + reused, active = _upload(tmp_path, key="file_second_upload", runner=runner) + + assert reused == keys + assert active.creates == 0 + assert _read(cache)["file_key"] == "file_stored"