From d5514da3abee45eea407c0e3602f574ef08836dc Mon Sep 17 00:00:00 2001 From: Joeseph Grey <212606152+StressTestor@users.noreply.github.com> Date: Sun, 16 Aug 2026 06:19:56 -0600 Subject: [PATCH] fix(tasks): scope action_tidy_research broken-file sweep to admins (#6069) action_tidy_research took an `owner` argument and never used it. Any user's scheduled tidy task swept data/deep_research globally, unlinking every empty or unparseable file regardless of who owned it. A broken file has no readable owner stamp, so it cannot be matched against `owner` the way _find_owned_research_path does, which is why the HTTP path and manage_research already treat parse failure as not-owned. Clearing one is a privileged act rather than an ownership one, so gate it on the canonical owner_is_admin_or_single_user helper: admins and the single-user operator keep the janitor, a regular user does not, and neither does the pre-setup window before an admin exists. Returns before the directory glob rather than filtering inside the loop, so a denied run reports why instead of reporting "none broken" over files it never inspected. That reason string surfaces in Activity as a skipped row. --- src/builtin_actions.py | 16 ++- tests/test_tidy_research_owner_scope.py | 159 ++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 tests/test_tidy_research_owner_scope.py diff --git a/src/builtin_actions.py b/src/builtin_actions.py index 216bb0360..5af3b4eca 100644 --- a/src/builtin_actions.py +++ b/src/builtin_actions.py @@ -810,13 +810,27 @@ async def action_tidy_research(owner: str, **kwargs) -> Tuple[str, bool]: Research history lives entirely in data/deep_research/.json and is NOT backed by chat-session rows — so a file must never be deleted just because - no chat session matches its id. Only prune files that fail to load.""" + no chat session matches its id. Only prune files that fail to load. + + A broken file has no readable owner stamp, so it cannot be matched against + `owner`. Clearing one is privileged: admins and the single-user operator + (AUTH_ENABLED=false) may, a regular user may not, and neither may anyone + during the pre-setup window before an admin exists. + """ try: from pathlib import Path import json as _json + from src.tool_security import owner_is_admin_or_single_user research_dir = Path(DEEP_RESEARCH_DIR) if not research_dir.exists(): raise TaskNoop("no research directory") + if not owner_is_admin_or_single_user(owner): + # Return before the glob rather than filtering inside the loop: the + # loop reports "none broken" off an empty `removed`, which reaches + # Activity as a false report to a user whose files it skipped, and a + # regular user need not read every owner's file to learn it may + # delete none of them. + raise TaskNoop("not permitted to remove unattributable research files") files = list(research_dir.glob("*.json")) removed = [] for p in files: diff --git a/tests/test_tidy_research_owner_scope.py b/tests/test_tidy_research_owner_scope.py new file mode 100644 index 000000000..3ecc78159 --- /dev/null +++ b/tests/test_tidy_research_owner_scope.py @@ -0,0 +1,159 @@ +"""Owner-scope tests for action_tidy_research. + +Broken research files (empty or unparseable JSON) have no readable owner +stamp. The HTTP path and _find_owned_research_path treat parse failure as +not-owned, so a regular user's tidy task must not unlink them. Clearing a +corrupt record is a privileged act instead: admins, and the operator in +single-user mode (AUTH_ENABLED=false), keep the janitor. + +Every case installs a fake AuthManager rather than relying on ambient state. +Setting AUTH_ENABLED=true alone leaves is_configured False, so the admin check +these tests exist to pin would never be reached. +""" + +import json + +import pytest + +from src.builtin_actions import TaskNoop, action_tidy_research + + +@pytest.fixture +def research_dir(tmp_path, monkeypatch): + data_dir = tmp_path / "deep_research" + data_dir.mkdir() + monkeypatch.setattr("src.builtin_actions.DEEP_RESEARCH_DIR", str(data_dir)) + return data_dir + + +def _install_auth(monkeypatch, *, is_configured: bool): + """Auth enabled, with `root` as the only admin. `is_configured` False models + the pre-setup window, where no admin exists yet.""" + + import core.auth as core_auth + + configured = is_configured + + class FakeAuthManager: + is_configured = configured + + def is_admin(self, user): + return user == "root" + + monkeypatch.setenv("AUTH_ENABLED", "true") + monkeypatch.setattr(core_auth, "AuthManager", FakeAuthManager) + + +@pytest.fixture +def configured_auth(monkeypatch): + _install_auth(monkeypatch, is_configured=True) + + +def _write(data_dir, name: str, text: str): + path = data_dir / f"{name}.json" + path.write_text(text, encoding="utf-8") + return path + + +def _write_owned(data_dir, name: str, owner: str): + return _write(data_dir, name, json.dumps({"owner": owner, "query": name})) + + +@pytest.mark.asyncio +async def test_configured_non_admin_skips_unparseable_and_empty_files( + research_dir, configured_auth +): + alice = _write_owned(research_dir, "alice-ok", "alice") + bob = _write_owned(research_dir, "bob-ok", "bob") + empty = _write(research_dir, "broken-empty", "") + garbage = _write(research_dir, "broken-json", "{not json") + alice_txt = alice.read_text(encoding="utf-8") + bob_txt = bob.read_text(encoding="utf-8") + + with pytest.raises(TaskNoop, match="not permitted"): + await action_tidy_research("alice") + + assert alice.exists() + assert alice.read_text(encoding="utf-8") == alice_txt + assert bob.exists() + assert bob.read_text(encoding="utf-8") == bob_txt + assert empty.exists(), "empty file has no owner stamp; non-admin tidy must not unlink it" + assert garbage.exists(), "unparseable file has no owner stamp; non-admin tidy must not unlink it" + + +@pytest.mark.asyncio +async def test_configured_non_admin_skip_reason_does_not_claim_none_broken( + research_dir, configured_auth +): + """The skip reason reaches Activity as `skipped -- `, so it must not + report a clean scan over files it declined to look at.""" + _write(research_dir, "broken-empty", "") + + with pytest.raises(TaskNoop) as excinfo: + await action_tidy_research("alice") + + assert "none broken" not in str(excinfo.value) + + +@pytest.mark.asyncio +async def test_configured_admin_still_removes_broken_files(research_dir, configured_auth): + valid = _write_owned(research_dir, "ok", "alice") + empty = _write(research_dir, "broken-empty", " ") + garbage = _write(research_dir, "broken-json", "not-json") + + message, ok = await action_tidy_research("root") + + assert ok is True + assert "Removed 2" in message + assert valid.exists(), "admin sweep clears corrupt records, not valid ones" + assert not empty.exists() + assert not garbage.exists() + + +@pytest.mark.asyncio +async def test_pre_setup_denies_owner_who_would_be_admin_once_configured( + research_dir, monkeypatch +): + """Auth on, no admin created yet: fail closed even for `root`. + + The owner here is non-empty and would satisfy is_admin, so this fails if the + is_configured guard is removed. An empty owner would not discriminate -- it + is denied by the `owner and ...` short-circuit either way. + """ + _install_auth(monkeypatch, is_configured=False) + empty = _write(research_dir, "broken-empty", "") + garbage = _write(research_dir, "broken-json", "{") + + with pytest.raises(TaskNoop, match="not permitted"): + await action_tidy_research("root") + + assert empty.exists() + assert garbage.exists() + + +@pytest.mark.asyncio +async def test_empty_owner_does_not_delete_broken_files(research_dir, configured_auth): + empty = _write(research_dir, "broken-empty", "") + garbage = _write(research_dir, "broken-json", "{") + + with pytest.raises(TaskNoop, match="not permitted"): + await action_tidy_research("") + + assert empty.exists() + assert garbage.exists() + + +@pytest.mark.asyncio +async def test_auth_disabled_still_removes_broken_files(research_dir, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + valid = _write_owned(research_dir, "ok", "alice") + empty = _write(research_dir, "broken-empty", " ") + garbage = _write(research_dir, "broken-json", "not-json") + + message, ok = await action_tidy_research("alice") + + assert ok is True + assert "Removed 2" in message + assert valid.exists() + assert not empty.exists() + assert not garbage.exists()