From 3f1348018746bdfe3c7fcf8f2fd24048ae7f22d9 Mon Sep 17 00:00:00 2001 From: Nanda Pranesh Date: Mon, 8 Jun 2026 18:35:45 +0530 Subject: [PATCH 01/10] WEB-4679: declare a scan manifest so the backend can prune uninstalled tools The discovery agent never told the backend which tools were still installed, so an uninstalled tool's row persisted on the dashboard forever. The completed scan event now carries a manifest of the (home_user, tool_name) pairs successfully scanned this run plus covered_home_users, letting the backend reconcile by set-difference and soft-delete what's gone. - ai_tools_discovery.py: accumulate scanned_manifest on the send-success and dedup hash-match paths only (a tool that errored on read is left out so it is never mistaken for uninstalled); pass manifest + covered_home_users (= all enumerated OS users, so a user who removed their last tool is still in scope) to the completed send_scan_event. - utils.py: send_scan_event gains optional manifest / covered_home_users, inserted into the payload only when present (backward compatible). Stdlib-only; the accumulation is pure in-memory and cannot raise. Pairs with the ai-gateway-data WEB-4679 reconcile change (forward/backward compatible: an old backend simply ignores the new fields). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 19 +- scripts/coding_discovery_tools/utils.py | 17 +- tests/test_scan_completed_manifest.py | 394 ++++++++++++++++++ 3 files changed, 427 insertions(+), 3 deletions(-) create mode 100644 tests/test_scan_completed_manifest.py diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index e7c6a4c..03b6a81 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2309,6 +2309,13 @@ def time_step(name: str, phase: str) -> Iterator[None]: # Track failed reports for persistence failed_reports = [] + # Manifest of (home_user, tool_name) pairs successfully read this run. + # Sent in the final "completed" event so the backend can set-diff against + # it to soft-delete (prune) tools that are no longer installed. A tool + # that errored on read is intentionally NOT recorded here, so a transient + # read failure is never mistaken for an uninstall. + scanned_manifest = [] + # --- Drain pending reports from previous run --- with time_step("drain_pending_queue", "queue"): pending = load_pending_reports() @@ -2619,6 +2626,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: if local_payload_hash and cached_hash == local_payload_hash: if not args.summary and not args.payload: logger.info(f" · {tool_name} unchanged for user {user_name} (hash match), skipping upload") + # Tool is still installed (just unchanged): record it so the + # backend does not prune it as uninstalled. Runs once per (tool, user). + scanned_manifest.append({"home_user": user_name, "tool_name": tool_name}) else: if not args.summary and not args.payload: logger.info(f" Sending {tool_name} report for user {user_name} to backend...") @@ -2630,6 +2640,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: logger.info(f" ✓ {tool_name} report for user {user_name} sent successfully") if local_payload_hash: discovery_cache.update_tool(tool_name, user_name, local_payload_hash) + # Successfully read and uploaded: record for the manifest so the + # backend does not prune it as uninstalled. Runs once per (tool, user). + scanned_manifest.append({"home_user": user_name, "tool_name": tool_name}) else: logger.error(f" ✗ Failed to send {tool_name} report for user {user_name} to backend") if retryable: @@ -2724,11 +2737,13 @@ def time_step(name: str, phase: str) -> Iterator[None]: except Exception as metrics_err: logger.debug(f"Building/sending discovery metrics failed: {metrics_err}") - # Send scan completed event AFTER all scanning + # Send scan completed event AFTER all scanning. Only this event carries the + # manifest and covered users, which the backend uses to prune uninstalled tools. logger.info("Sending scan completed event...") success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", - args.app_name, sentry_context=sentry_ctx + args.app_name, sentry_context=sentry_ctx, + manifest=scanned_manifest, covered_home_users=all_users ) if success: logger.info("✓ Scan completed event sent successfully") diff --git a/scripts/coding_discovery_tools/utils.py b/scripts/coding_discovery_tools/utils.py index 7b09014..3bf77d8 100644 --- a/scripts/coding_discovery_tools/utils.py +++ b/scripts/coding_discovery_tools/utils.py @@ -485,7 +485,9 @@ def send_scan_event( app_name: Optional[str] = None, home_user: Optional[str] = None, scan_error: Optional[Dict] = None, - sentry_context: Optional[Dict] = None + sentry_context: Optional[Dict] = None, + manifest: Optional[List[Dict]] = None, + covered_home_users: Optional[List[str]] = None ) -> Tuple[bool, bool]: """ Send scan lifecycle event to backend (in_progress, completed, failed). @@ -500,6 +502,13 @@ def send_scan_event( home_user: Optional user context (for user-specific failures) scan_error: Optional error data (required when scan_event="failed") sentry_context: Optional context dict forwarded to Sentry on failure + manifest: Optional list of {"home_user", "tool_name"} dicts that were + successfully scanned this run. Sent only on the "completed" event so + the backend can set-diff against it to soft-delete (prune) tools that + are no longer installed. Tools that errored on read are excluded. + covered_home_users: Optional list of home users this run enumerated. Sent + only on the "completed" event; bounds the prune to the covered scope + (and includes a user whose last tool was removed). Returns: Tuple of (success, retryable): success=True if sent, retryable=True if caller should queue @@ -519,6 +528,12 @@ def send_scan_event( if scan_error: payload["scan_error"] = scan_error + if manifest is not None: + payload["manifest"] = manifest + + if covered_home_users is not None: + payload["covered_home_users"] = covered_home_users + return send_report_to_backend( backend_url, api_key, diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py new file mode 100644 index 0000000..04aeaf5 --- /dev/null +++ b/tests/test_scan_completed_manifest.py @@ -0,0 +1,394 @@ +""" +Tests for WEB-4679: the scan "completed" event carries a manifest of the +(home_user, tool_name) pairs that were actually read this run, plus the full +set of enumerated home users, so the backend can set-diff and soft-delete +(prune) tools that are no longer installed. + +Correctness property under test (the load-bearing one): a tool whose read +ERRORED is NEVER recorded in the manifest, so a transient read failure can +never be mistaken for an uninstall. Tools that were sent successfully OR +short-circuited by the local hash-match dedup (still installed, just unchanged) +ARE recorded. + +Seams (mirroring the existing suite in test_send_and_persist.py / +test_discovery_flow.py): + * TestSendScanEventManifest -> utils.send_scan_event against a real + localhost HTTP server (records POST bodies). Covers the payload-shaping + contract + backward compatibility. + * TestCompletedEventManifestCLI -> main() via subprocess against a real + localhost HTTP server. Covers the end-to-end completed-event payload and + that in_progress/failed events do NOT carry a manifest. + * TestManifestExcludesErroredReads -> main() driven IN-PROCESS with a mock + detector so a per-tool read error / hash-match / success can be forced + deterministically. This is the only seam where the errored-read branch can + be isolated, because the per-(tool, user) loop body lives inline in main(). + +Only external environment is mocked: HTTP backend (real server on localhost), +HOME (so the subprocess gets an isolated discovery lock/cache and never exits +early on a live lock from another run), _SENTRY_DSN (no real Sentry calls), +discovery_cache / detector (in-process seam only). No network is required. +""" + +import json +import os +import subprocess +import sys +import tempfile +import threading +import unittest +from http.server import HTTPServer, BaseHTTPRequestHandler +from pathlib import Path +from unittest.mock import Mock, patch + +import scripts.coding_discovery_tools.utils as utils_mod +from scripts.coding_discovery_tools.utils import send_scan_event + +REPO_ROOT = Path(__file__).resolve().parent.parent + + +class _RecordingHandler(BaseHTTPRequestHandler): + """Records every POST body (parsed as JSON) and returns 200.""" + + def do_POST(self): + length = int(self.headers.get("Content-Length", 0)) + body = self.rfile.read(length) + try: + self.server.requests.append(json.loads(body)) + except ValueError: + self.server.requests.append({"_raw": body.decode("utf-8", "replace")}) + + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(b'{"ok": true}') + + def log_message(self, format, *args): + pass # suppress server logs + + +class _ServerTestCase(unittest.TestCase): + """Spins up one recording HTTP server on localhost for the whole class.""" + + @classmethod + def setUpClass(cls): + cls.server = HTTPServer(("127.0.0.1", 0), _RecordingHandler) + cls.server.requests = [] + cls.port = cls.server.server_address[1] + cls.base_url = f"http://127.0.0.1:{cls.port}" + cls.thread = threading.Thread(target=cls.server.serve_forever) + cls.thread.daemon = True + cls.thread.start() + + @classmethod + def tearDownClass(cls): + cls.server.shutdown() + cls.thread.join(timeout=5) + + def setUp(self): + self.server.requests.clear() + + +class TestSendScanEventManifest(_ServerTestCase): + """utils.send_scan_event seam: manifest + covered_home_users are inserted + into the POST body only when provided (backward compatible).""" + + @patch("time.sleep") + @patch.object(utils_mod, "_SENTRY_DSN", "") + def test_completed_event_carries_manifest_and_covered_users(self, _sleep): + manifest = [{"home_user": "alice", "tool_name": "Cursor"}] + covered = ["alice", "bob"] + + success, _retryable = send_scan_event( + self.base_url, + "test-key", + "DEV-1", + "run-1", + "completed", + manifest=manifest, + covered_home_users=covered, + ) + + self.assertTrue(success) + self.assertEqual(len(self.server.requests), 1) + body = self.server.requests[0] + # Exact passthrough of both new fields. + self.assertEqual(body["scan_event"], "completed") + self.assertEqual(body["manifest"], manifest) + self.assertEqual(body["covered_home_users"], covered) + + @patch("time.sleep") + @patch.object(utils_mod, "_SENTRY_DSN", "") + def test_legacy_call_omits_both_keys(self, _sleep): + # No manifest / covered_home_users supplied -> neither key may appear + # in the payload (backward compatibility with the old call sites). + success, _retryable = send_scan_event( + self.base_url, "test-key", "DEV-1", "run-1", "in_progress" + ) + + self.assertTrue(success) + self.assertEqual(len(self.server.requests), 1) + body = self.server.requests[0] + self.assertNotIn("manifest", body) + self.assertNotIn("covered_home_users", body) + + @patch("time.sleep") + @patch.object(utils_mod, "_SENTRY_DSN", "") + def test_empty_manifest_still_sent(self, _sleep): + # An empty manifest is meaningfully different from "no manifest": it + # tells the backend "this scope had zero readable tools" (prune-all + # within scope). It must be sent (key present), since the production + # guard is `is not None`, not truthiness. + success, _retryable = send_scan_event( + self.base_url, + "test-key", + "DEV-1", + "run-1", + "completed", + manifest=[], + covered_home_users=["alice"], + ) + + self.assertTrue(success) + body = self.server.requests[0] + self.assertIn("manifest", body) + self.assertEqual(body["manifest"], []) + self.assertEqual(body["covered_home_users"], ["alice"]) + + +class TestCompletedEventManifestCLI(_ServerTestCase): + """End-to-end via main() subprocess: the completed event carries a + well-formed manifest + covered_home_users; lifecycle events that are not + "completed" carry neither.""" + + def _run_cli(self, timeout=600): + env = os.environ.copy() + # Isolate the discovery state dir (lock + cache) under a throwaway HOME + # so the run never exits early on a live lock left by another process, + # and starts from a cold cache (deterministic hash-match behavior). + env["HOME"] = tempfile.mkdtemp(prefix="web4679_home_") + return subprocess.run( + [ + sys.executable, + "scripts/coding_discovery_tools/ai_tools_discovery.py", + "--api-key", + "test-key-000000", + "--domain", + self.base_url, + ], + cwd=str(REPO_ROOT), + capture_output=True, + text=True, + timeout=timeout, + env=env, + ) + + def test_completed_event_has_manifest_and_covered_users(self): + result = self._run_cli() + self.assertEqual(result.returncode, 0, f"stderr: {result.stderr[-2000:]}") + + completed = [ + r for r in self.server.requests if r.get("scan_event") == "completed" + ] + self.assertEqual(len(completed), 1, "expected exactly one completed event") + body = completed[0] + + # manifest: list of {home_user, tool_name} objects. + self.assertIn("manifest", body) + self.assertIsInstance(body["manifest"], list) + for entry in body["manifest"]: + self.assertIsInstance(entry, dict) + self.assertIn("home_user", entry) + self.assertIn("tool_name", entry) + self.assertIsInstance(entry["home_user"], str) + self.assertIsInstance(entry["tool_name"], str) + + # covered_home_users: list of user names (strings). + self.assertIn("covered_home_users", body) + self.assertIsInstance(body["covered_home_users"], list) + for user in body["covered_home_users"]: + self.assertIsInstance(user, str) + + def test_non_completed_events_have_no_manifest(self): + result = self._run_cli() + self.assertEqual(result.returncode, 0, f"stderr: {result.stderr[-2000:]}") + + # An in_progress event is always sent before scanning. + non_completed = [ + r + for r in self.server.requests + if r.get("scan_event") in ("in_progress", "failed") + ] + self.assertGreaterEqual( + len(non_completed), 1, "expected at least an in_progress event" + ) + for body in non_completed: + self.assertNotIn( + "manifest", body, f"{body.get('scan_event')} must not carry a manifest" + ) + self.assertNotIn( + "covered_home_users", + body, + f"{body.get('scan_event')} must not carry covered_home_users", + ) + + def test_covered_home_users_matches_full_enumeration_not_manifest(self): + # covered_home_users is sourced from the full user enumeration + # (all_users), NOT only from users that produced manifest entries. So a + # user who contributed zero manifest entries still appears in + # covered_home_users. We assert this invariant without needing to force + # a specific zero-tool user on the host: every home_user that appears in + # the manifest must also appear in covered_home_users, and + # covered_home_users must be a superset of the manifest's user set. + result = self._run_cli() + self.assertEqual(result.returncode, 0, f"stderr: {result.stderr[-2000:]}") + + completed = [ + r for r in self.server.requests if r.get("scan_event") == "completed" + ] + self.assertEqual(len(completed), 1) + body = completed[0] + + covered = set(body["covered_home_users"]) + manifest_users = {e["home_user"] for e in body["manifest"]} + # The enumerated set must cover every user that yielded a manifest entry. + self.assertTrue( + manifest_users.issubset(covered), + f"manifest users {manifest_users} not all in covered {covered}", + ) + + +class TestManifestExcludesErroredReads(unittest.TestCase): + """The load-bearing property: a tool whose read raises is OMITTED from the + manifest, while a successfully-sent tool and a hash-match (unchanged, still + installed) tool are BOTH included. + + Seam: main() driven in-process with a mocked detector + discovery_cache and + a captured send_scan_event. This is necessary because the per-(tool, user) + loop body — with its success / hash-match / PermissionError branches — lives + inline inside main() and is not an independently callable unit. No + production code was changed to enable this; every name patched here is a + module-level import already present in ai_tools_discovery. + """ + + def setUp(self): + import scripts.coding_discovery_tools.ai_tools_discovery as adm + + self.adm = adm + self.argv = [ + "ai_tools_discovery.py", + "--api-key", + "k", + "--domain", + "http://127.0.0.1:1", + ] + + @staticmethod + def _make_tool(name): + # Distinct install_path per tool so the (name:path) dedup keeps all three. + return {"name": name, "version": "1.0", "install_path": f"/opt/{name}", "projects": []} + + def _run_main_capture_manifest(self): + """Run main() with three crafted tools for one user: + ToolOK -> hash mismatch -> send path -> send succeeds -> appended + ToolHashMatch -> hash match -> dedup short-circuit -> appended + ToolErr -> filter raises PermissionError -> NOT appended + Returns the captured (manifest, covered_home_users) from the completed + send_scan_event call. + """ + adm = self.adm + + tool_ok = self._make_tool("ToolOK") + tool_hm = self._make_tool("ToolHashMatch") + tool_err = self._make_tool("ToolErr") + + detector = Mock() + detector.get_device_id.return_value = "dev-xyz" + detector.detect_all_tools.return_value = [tool_ok, tool_hm, tool_err] + detector._set_canonical_vscode_copilot.return_value = None + detector.process_single_tool.side_effect = lambda t: t + + def _filter(tool_with_projects, _user_home): + if tool_with_projects["name"] == "ToolErr": + raise PermissionError("simulated read failure") + return tool_with_projects + + detector.filter_tool_projects_by_user.side_effect = _filter + detector.generate_single_tool_report.side_effect = ( + lambda tool, device_id, home_user, system_user=None, run_id=None: { + "tools": [tool] + } + ) + + # Hash is derived from the tool name; the cache "matches" only for + # ToolHashMatch, forcing ToolOK down the send path and ToolHashMatch + # down the dedup short-circuit. + def _hash(tool_dict): + return "hash-" + tool_dict["name"] + + def _cached(tool_name, _user_name): + return "hash-ToolHashMatch" if tool_name == "ToolHashMatch" else None + + dc = Mock() + dc.acquire_lock.return_value = "acquired" + dc.heartbeat_start.return_value = Mock() + dc.get_cached_hash.side_effect = _cached + dc.update_tool.return_value = None + dc.UNBOUND_DIR = "/tmp/unbound-test" + dc.last_lock_error = None + + captured = {} + + def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=None, **kw): + if scan_event == "completed": + captured["manifest"] = kw.get("manifest") + captured["covered_home_users"] = kw.get("covered_home_users") + return (True, None) + + with patch.object(adm.platform, "system", return_value="Darwin"), \ + patch.object(adm, "AIToolsDetector", return_value=detector), \ + patch.object(adm, "discovery_cache", dc), \ + patch.object(adm, "get_all_users_macos", return_value=["alice"]), \ + patch.object(adm, "compute_payload_hash", side_effect=_hash), \ + patch.object(adm, "send_report_to_backend", return_value=(True, False)), \ + patch.object(adm, "send_scan_event", side_effect=_send_scan_event), \ + patch.object(adm, "send_discovery_metrics", Mock()), \ + patch.object(adm, "load_pending_reports", return_value=[]), \ + patch.object(adm, "save_failed_reports", Mock()), \ + patch.object(adm, "report_to_sentry", Mock()), \ + patch.object(utils_mod, "_SENTRY_DSN", ""), \ + patch.object(sys, "argv", self.argv): + try: + adm.main() + except SystemExit: + pass + + return captured + + def test_errored_tool_excluded_success_and_hashmatch_included(self): + captured = self._run_main_capture_manifest() + + self.assertIn("manifest", captured, "completed event was never sent") + pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} + + # Success path recorded. + self.assertIn(("alice", "ToolOK"), pairs) + # Hash-match (unchanged, still installed) recorded. + self.assertIn(("alice", "ToolHashMatch"), pairs) + # Errored read is the load-bearing exclusion: a read failure must never + # look like an uninstall. + self.assertNotIn(("alice", "ToolErr"), pairs) + # Exactly the two non-errored pairs, nothing else. + self.assertEqual(len(captured["manifest"]), 2) + + def test_covered_home_users_includes_user_with_no_manifest_entry(self): + # covered_home_users must come from the full enumeration (all_users), + # so even though "alice" is the only user and one of her tools errored, + # she still appears. More importantly, this proves covered_home_users is + # not derived from the manifest: a user whose every tool errored would + # still be covered (bounding the prune scope correctly). + captured = self._run_main_capture_manifest() + self.assertEqual(captured.get("covered_home_users"), ["alice"]) + + +if __name__ == "__main__": + unittest.main() From ff39524ac47eaf5e5b92c24ae22b86bfaf5ad356 Mon Sep 17 00:00:00 2001 From: Nanda Pranesh Date: Mon, 8 Jun 2026 19:08:42 +0530 Subject: [PATCH 02/10] WEB-4679: dedupe scan manifest via a set of (home_user, tool_name) Addresses Greptile P2: accumulate the manifest as a set of tuples (a pair can never be double-recorded) and serialize to a sorted list of dicts at the send site. Functionally equivalent today (the success and hash-match branches are mutually exclusive, one entry per (tool, user)), but removes the latent duplicate risk and makes the output deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/coding_discovery_tools/ai_tools_discovery.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index 03b6a81..9a8c55b 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2313,8 +2313,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: # Sent in the final "completed" event so the backend can set-diff against # it to soft-delete (prune) tools that are no longer installed. A tool # that errored on read is intentionally NOT recorded here, so a transient - # read failure is never mistaken for an uninstall. - scanned_manifest = [] + # read failure is never mistaken for an uninstall. A set keyed on + # (home_user, tool_name) so a pair can never be double-recorded. + scanned_manifest = set() # --- Drain pending reports from previous run --- with time_step("drain_pending_queue", "queue"): @@ -2628,7 +2629,7 @@ def time_step(name: str, phase: str) -> Iterator[None]: logger.info(f" · {tool_name} unchanged for user {user_name} (hash match), skipping upload") # Tool is still installed (just unchanged): record it so the # backend does not prune it as uninstalled. Runs once per (tool, user). - scanned_manifest.append({"home_user": user_name, "tool_name": tool_name}) + scanned_manifest.add((user_name, tool_name)) else: if not args.summary and not args.payload: logger.info(f" Sending {tool_name} report for user {user_name} to backend...") @@ -2642,7 +2643,7 @@ def time_step(name: str, phase: str) -> Iterator[None]: discovery_cache.update_tool(tool_name, user_name, local_payload_hash) # Successfully read and uploaded: record for the manifest so the # backend does not prune it as uninstalled. Runs once per (tool, user). - scanned_manifest.append({"home_user": user_name, "tool_name": tool_name}) + scanned_manifest.add((user_name, tool_name)) else: logger.error(f" ✗ Failed to send {tool_name} report for user {user_name} to backend") if retryable: @@ -2740,10 +2741,11 @@ def time_step(name: str, phase: str) -> Iterator[None]: # Send scan completed event AFTER all scanning. Only this event carries the # manifest and covered users, which the backend uses to prune uninstalled tools. logger.info("Sending scan completed event...") + manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", args.app_name, sentry_context=sentry_ctx, - manifest=scanned_manifest, covered_home_users=all_users + manifest=manifest, covered_home_users=all_users ) if success: logger.info("✓ Scan completed event sent successfully") From a2087ba0bb340a23c0665d96023b11d400c4a833 Mon Sep 17 00:00:00 2001 From: Nanda Pranesh Date: Mon, 8 Jun 2026 20:04:43 +0530 Subject: [PATCH 03/10] WEB-4679: keep read-success tools in the manifest on upload failure; tighten comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flag: a tool whose READ succeeded but whose upload failed transiently was dropped from the manifest, so the backend could mistake a network blip for an uninstall and prune a live tool (enforce mode). Record it in the send-failure branch too — the manifest tracks what was SEEN, not what uploaded. Adds a regression test (read-success + upload-failure stays in the manifest). Also condense the WEB-4679 comments to concise one-liners. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 18 ++++------ scripts/coding_discovery_tools/utils.py | 11 +++--- tests/test_scan_completed_manifest.py | 34 ++++++++++++++++--- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index 9a8c55b..de3c3fb 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2309,12 +2309,7 @@ def time_step(name: str, phase: str) -> Iterator[None]: # Track failed reports for persistence failed_reports = [] - # Manifest of (home_user, tool_name) pairs successfully read this run. - # Sent in the final "completed" event so the backend can set-diff against - # it to soft-delete (prune) tools that are no longer installed. A tool - # that errored on read is intentionally NOT recorded here, so a transient - # read failure is never mistaken for an uninstall. A set keyed on - # (home_user, tool_name) so a pair can never be double-recorded. + # (home_user, tool_name) tools seen this run; backend set-diffs it in "completed" to prune the rest. scanned_manifest = set() # --- Drain pending reports from previous run --- @@ -2627,8 +2622,7 @@ def time_step(name: str, phase: str) -> Iterator[None]: if local_payload_hash and cached_hash == local_payload_hash: if not args.summary and not args.payload: logger.info(f" · {tool_name} unchanged for user {user_name} (hash match), skipping upload") - # Tool is still installed (just unchanged): record it so the - # backend does not prune it as uninstalled. Runs once per (tool, user). + # unchanged but still installed -> record so the backend won't prune it scanned_manifest.add((user_name, tool_name)) else: if not args.summary and not args.payload: @@ -2641,13 +2635,14 @@ def time_step(name: str, phase: str) -> Iterator[None]: logger.info(f" ✓ {tool_name} report for user {user_name} sent successfully") if local_payload_hash: discovery_cache.update_tool(tool_name, user_name, local_payload_hash) - # Successfully read and uploaded: record for the manifest so the - # backend does not prune it as uninstalled. Runs once per (tool, user). + # read + uploaded -> record so the backend won't prune it scanned_manifest.add((user_name, tool_name)) else: logger.error(f" ✗ Failed to send {tool_name} report for user {user_name} to backend") if retryable: failed_reports.append(single_tool_report) + # read OK, only upload failed (retried later) -> still installed, so record it (manifest = seen, not uploaded) + scanned_manifest.add((user_name, tool_name)) if not args.summary and not args.payload: logger.info("") @@ -2738,8 +2733,7 @@ def time_step(name: str, phase: str) -> Iterator[None]: except Exception as metrics_err: logger.debug(f"Building/sending discovery metrics failed: {metrics_err}") - # Send scan completed event AFTER all scanning. Only this event carries the - # manifest and covered users, which the backend uses to prune uninstalled tools. + # only the completed event carries manifest + covered users (backend prunes from them) logger.info("Sending scan completed event...") manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] success, _ = send_scan_event( diff --git a/scripts/coding_discovery_tools/utils.py b/scripts/coding_discovery_tools/utils.py index 3bf77d8..644b9a9 100644 --- a/scripts/coding_discovery_tools/utils.py +++ b/scripts/coding_discovery_tools/utils.py @@ -502,13 +502,10 @@ def send_scan_event( home_user: Optional user context (for user-specific failures) scan_error: Optional error data (required when scan_event="failed") sentry_context: Optional context dict forwarded to Sentry on failure - manifest: Optional list of {"home_user", "tool_name"} dicts that were - successfully scanned this run. Sent only on the "completed" event so - the backend can set-diff against it to soft-delete (prune) tools that - are no longer installed. Tools that errored on read are excluded. - covered_home_users: Optional list of home users this run enumerated. Sent - only on the "completed" event; bounds the prune to the covered scope - (and includes a user whose last tool was removed). + manifest: Optional [{"home_user", "tool_name"}] seen this run; sent only on + "completed" so the backend set-diffs it to prune the rest. + covered_home_users: Optional home users covered; sent only on "completed" to + bound the prune scope. Returns: Tuple of (success, retryable): success=True if sent, retryable=True if caller should queue diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 04aeaf5..257701c 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -287,11 +287,13 @@ def _make_tool(name): # Distinct install_path per tool so the (name:path) dedup keeps all three. return {"name": name, "version": "1.0", "install_path": f"/opt/{name}", "projects": []} - def _run_main_capture_manifest(self): + def _run_main_capture_manifest(self, send_report_result=(True, False)): """Run main() with three crafted tools for one user: - ToolOK -> hash mismatch -> send path -> send succeeds -> appended - ToolHashMatch -> hash match -> dedup short-circuit -> appended - ToolErr -> filter raises PermissionError -> NOT appended + ToolOK -> hash mismatch -> send path -> send result varies -> appended + ToolHashMatch -> hash match -> dedup short-circuit -> appended + ToolErr -> filter raises PermissionError -> NOT appended + send_report_result controls send_report_to_backend's (success, retryable) + return — pass (False, True) to exercise a read-success / upload-failure. Returns the captured (manifest, covered_home_users) from the completed send_scan_event call. """ @@ -349,7 +351,7 @@ def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=No patch.object(adm, "discovery_cache", dc), \ patch.object(adm, "get_all_users_macos", return_value=["alice"]), \ patch.object(adm, "compute_payload_hash", side_effect=_hash), \ - patch.object(adm, "send_report_to_backend", return_value=(True, False)), \ + patch.object(adm, "send_report_to_backend", return_value=send_report_result), \ patch.object(adm, "send_scan_event", side_effect=_send_scan_event), \ patch.object(adm, "send_discovery_metrics", Mock()), \ patch.object(adm, "load_pending_reports", return_value=[]), \ @@ -380,6 +382,28 @@ def test_errored_tool_excluded_success_and_hashmatch_included(self): # Exactly the two non-errored pairs, nothing else. self.assertEqual(len(captured["manifest"]), 2) + def test_read_success_but_upload_failure_still_in_manifest(self): + # A tool whose READ succeeds but whose UPLOAD fails transiently + # (send_report_to_backend -> (False, retryable=True)) is still installed. + # It MUST stay in the manifest; otherwise the backend would mistake a + # transient upload failure for an uninstall and prune a live tool. This + # fails before the fix (ToolOK absent when its upload fails) and passes + # after (the manifest tracks what was SEEN, not what uploaded). + captured = self._run_main_capture_manifest(send_report_result=(False, True)) + + self.assertIn("manifest", captured, "completed event was never sent") + pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} + + # Read-success tool whose upload FAILED is still recorded. + self.assertIn(("alice", "ToolOK"), pairs) + # Hash-match (no upload at all) still recorded. + self.assertIn(("alice", "ToolHashMatch"), pairs) + # An errored READ remains excluded — that path is unchanged. + self.assertNotIn(("alice", "ToolErr"), pairs) + # Manifest is identical to the all-uploads-succeed case: read-success is + # what counts, not upload success. + self.assertEqual(len(captured["manifest"]), 2) + def test_covered_home_users_includes_user_with_no_manifest_entry(self): # covered_home_users must come from the full enumeration (all_users), # so even though "alice" is the only user and one of her tools errored, From f6987a60fa3da665e0fd3df90623da9dafebf585 Mon Sep 17 00:00:00 2001 From: audit Date: Fri, 12 Jun 2026 19:39:55 +0530 Subject: [PATCH 04/10] =?UTF-8?q?WEB-4679:=20fail=20closed=20=E2=80=94=20m?= =?UTF-8?q?anifest=3DNone=20when=20a=20read=20error=20may=20omit=20a=20liv?= =?UTF-8?q?e=20tool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool whose read errors without a scan_event=failed (the generic per-user except, the per-tool except, or a PermissionError whose failed-event send itself fails) leaves the manifest possibly missing an installed tool — the backend would then set-diff it as uninstalled and wrongly prune it. Track scanned_manifest_complete and send manifest=None (backend treats as legacy = no prune) when the run wasn't fully read, deferring cleanup to a clean run. Adds a test forcing a generic (non-Permission) read error -> manifest=None. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 18 ++++++++++++++++- tests/test_scan_completed_manifest.py | 20 +++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index de3c3fb..9bd4d0a 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2311,6 +2311,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: # (home_user, tool_name) tools seen this run; backend set-diffs it in "completed" to prune the rest. scanned_manifest = set() + # Fail closed: if a read error below omits a live tool from the manifest, send manifest=None + # (backend treats as legacy = no prune) rather than risk wrongly deleting an installed tool. + scanned_manifest_complete = True # --- Drain pending reports from previous run --- with time_step("drain_pending_queue", "queue"): @@ -2666,12 +2669,17 @@ def time_step(name: str, phase: str) -> Iterator[None]: ) if not success: logger.warning("✗ Failed to send scan failed event") + # Backend won't see this scan as unclean -> don't let it prune from a partial manifest. + scanned_manifest_complete = False report_to_sentry(e, {**sentry_ctx, "phase": "process_tool_user", "tool_name": tool_name, "user": user_name}, level="warning") logger.info("") except Exception as e: logger.error(f"Error processing {tool_name} for user {user_name}: {e}", exc_info=True) + # No scan_event=failed is sent here, so the backend can't see this gap; + # mark the manifest incomplete so the completed event sends manifest=None. + scanned_manifest_complete = False report_to_sentry(e, {**sentry_ctx, "phase": "process_tool_user", "tool_name": tool_name}, level="warning") logger.info("") @@ -2688,6 +2696,8 @@ def time_step(name: str, phase: str) -> Iterator[None]: except Exception as e: logger.error(f"Error processing tool {tool_name}: {e}", exc_info=True) + # Tool omitted device-wide with no failed event -> manifest is incomplete. + scanned_manifest_complete = False report_to_sentry(e, {**sentry_ctx, "phase": "process_tool", "tool_name": tool_name}, level="warning") logger.info("") @@ -2735,7 +2745,13 @@ def time_step(name: str, phase: str) -> Iterator[None]: # only the completed event carries manifest + covered users (backend prunes from them) logger.info("Sending scan completed event...") - manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] + # Fail closed: if any tool/user read errored without a scan_event=failed, the manifest may be + # missing a live tool -> send manifest=None so the backend treats this run as legacy (no prune). + if scanned_manifest_complete: + manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] + else: + manifest = None + logger.warning("Scan had read errors; sending manifest=None to skip pruning this run") success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", args.app_name, sentry_context=sentry_ctx, diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 257701c..1e37dd2 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -287,7 +287,7 @@ def _make_tool(name): # Distinct install_path per tool so the (name:path) dedup keeps all three. return {"name": name, "version": "1.0", "install_path": f"/opt/{name}", "projects": []} - def _run_main_capture_manifest(self, send_report_result=(True, False)): + def _run_main_capture_manifest(self, send_report_result=(True, False), filter_error=None): """Run main() with three crafted tools for one user: ToolOK -> hash mismatch -> send path -> send result varies -> appended ToolHashMatch -> hash match -> dedup short-circuit -> appended @@ -311,7 +311,7 @@ def _run_main_capture_manifest(self, send_report_result=(True, False)): def _filter(tool_with_projects, _user_home): if tool_with_projects["name"] == "ToolErr": - raise PermissionError("simulated read failure") + raise (filter_error if filter_error is not None else PermissionError("simulated read failure")) return tool_with_projects detector.filter_tool_projects_by_user.side_effect = _filter @@ -413,6 +413,22 @@ def test_covered_home_users_includes_user_with_no_manifest_entry(self): captured = self._run_main_capture_manifest() self.assertEqual(captured.get("covered_home_users"), ["alice"]) + def test_generic_read_error_fails_closed_manifest_none(self): + # A NON-Permission read error sends NO scan_event=failed, so the backend can't tell the + # scan was unclean. The completed event must fail closed: manifest=None (legacy = no prune), + # so a transient error can never drive a wrongful device-wide delete. (A PermissionError, by + # contrast, DOES send a failed event and keeps a real manifest — covered by the test above.) + captured = self._run_main_capture_manifest( + filter_error=RuntimeError("simulated generic read failure") + ) + self.assertIn("manifest", captured, "completed event was never sent") + self.assertIsNone( + captured["manifest"], + "a generic (non-Permission) read error must fail closed -> manifest=None", + ) + # covered_home_users is still reported (telemetry of who was enumerated). + self.assertEqual(captured.get("covered_home_users"), ["alice"]) + if __name__ == "__main__": unittest.main() From bcb482e261ece3d18019d1e833bfea289b908164 Mon Sep 17 00:00:00 2001 From: audit Date: Fri, 26 Jun 2026 23:24:16 +0530 Subject: [PATCH 05/10] WEB-4679: build scan manifest from detection/presence, not extraction success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A read/extraction error no longer drops a live tool from the manifest (presence is recorded before extraction) and never fail-closes the whole manifest to None — so one tool's failure can't block pruning every other tool on the device. A detector that errors is kept in the manifest (presence unknown, not an uninstall). Removes the global scanned_manifest_complete fail-close. Tests rewritten to the new contract. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 48 ++++--- tests/test_scan_completed_manifest.py | 122 +++++++++--------- 2 files changed, 84 insertions(+), 86 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index 9bd4d0a..cdddd74 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -279,7 +279,7 @@ def get_device_id(self) -> str: """ return self._device_id_extractor.extract_device_id() - def detect_all_tools(self, user_home: Optional[Path] = None) -> List[Dict]: + def detect_all_tools(self, user_home: Optional[Path] = None, failures: Optional[set] = None) -> List[Dict]: """ Detect all supported AI tools. @@ -310,6 +310,9 @@ def detect_all_tools(self, user_home: Optional[Path] = None) -> List[Dict]: except Exception as e: logger.warning(f"Error detecting {detector.tool_name}: {e}") report_to_sentry(e, {"phase": "detect", "tool_name": detector.tool_name}, level="warning") + # Detection errored: record the tool so the caller can keep it (presence unknown != uninstalled). + if failures is not None: + failures.add(detector.tool_name) return tools @@ -2309,11 +2312,8 @@ def time_step(name: str, phase: str) -> Iterator[None]: # Track failed reports for persistence failed_reports = [] - # (home_user, tool_name) tools seen this run; backend set-diffs it in "completed" to prune the rest. + # (home_user, tool_name) detected present this run; backend set-diffs it in "completed" to prune the rest. scanned_manifest = set() - # Fail closed: if a read error below omits a live tool from the manifest, send manifest=None - # (backend treats as legacy = no prune) rather than risk wrongly deleting an installed tool. - scanned_manifest_complete = True # --- Drain pending reports from previous run --- with time_step("drain_pending_queue", "queue"): @@ -2398,7 +2398,13 @@ def time_step(name: str, phase: str) -> Iterator[None]: user_home = Path.home() logger.info(f" Detecting tools for user: {user} (home: {user_home})") with time_step("detect_tools", "detect"): - user_tools = detector.detect_all_tools(user_home=user_home) + user_detect_failures = set() + user_tools = detector.detect_all_tools( + user_home=user_home, failures=user_detect_failures + ) + # Detector errored -> presence unknown -> keep it in the manifest (don't treat as uninstalled). + for failed_tool_name in user_detect_failures: + scanned_manifest.add((user, failed_tool_name)) if user_tools: logger.info(f" Found {len(user_tools)} tool(s) for {user}:") @@ -2459,6 +2465,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: user_home = Path.home() try: + # Record presence before extraction so a read error below can't drop a live tool. + scanned_manifest.add((user_name, tool_name)) + # Filter projects to only include this user's projects with time_step("filter_projects", "process"): tool_filtered = detector.filter_tool_projects_by_user(tool_with_projects, user_home) @@ -2474,6 +2483,8 @@ def time_step(name: str, phase: str) -> Iterator[None]: f"{tool_filtered.get('install_path')!r} not owned by this " f"user and no per-user data" ) + # Not actually present for this user -> undo the optimistic presence record. + scanned_manifest.discard((user_name, tool_name)) continue # Detect subscription plan for Claude Code @@ -2625,8 +2636,6 @@ def time_step(name: str, phase: str) -> Iterator[None]: if local_payload_hash and cached_hash == local_payload_hash: if not args.summary and not args.payload: logger.info(f" · {tool_name} unchanged for user {user_name} (hash match), skipping upload") - # unchanged but still installed -> record so the backend won't prune it - scanned_manifest.add((user_name, tool_name)) else: if not args.summary and not args.payload: logger.info(f" Sending {tool_name} report for user {user_name} to backend...") @@ -2638,14 +2647,10 @@ def time_step(name: str, phase: str) -> Iterator[None]: logger.info(f" ✓ {tool_name} report for user {user_name} sent successfully") if local_payload_hash: discovery_cache.update_tool(tool_name, user_name, local_payload_hash) - # read + uploaded -> record so the backend won't prune it - scanned_manifest.add((user_name, tool_name)) else: logger.error(f" ✗ Failed to send {tool_name} report for user {user_name} to backend") if retryable: failed_reports.append(single_tool_report) - # read OK, only upload failed (retried later) -> still installed, so record it (manifest = seen, not uploaded) - scanned_manifest.add((user_name, tool_name)) if not args.summary and not args.payload: logger.info("") @@ -2669,17 +2674,12 @@ def time_step(name: str, phase: str) -> Iterator[None]: ) if not success: logger.warning("✗ Failed to send scan failed event") - # Backend won't see this scan as unclean -> don't let it prune from a partial manifest. - scanned_manifest_complete = False report_to_sentry(e, {**sentry_ctx, "phase": "process_tool_user", "tool_name": tool_name, "user": user_name}, level="warning") logger.info("") except Exception as e: logger.error(f"Error processing {tool_name} for user {user_name}: {e}", exc_info=True) - # No scan_event=failed is sent here, so the backend can't see this gap; - # mark the manifest incomplete so the completed event sends manifest=None. - scanned_manifest_complete = False report_to_sentry(e, {**sentry_ctx, "phase": "process_tool_user", "tool_name": tool_name}, level="warning") logger.info("") @@ -2696,8 +2696,9 @@ def time_step(name: str, phase: str) -> Iterator[None]: except Exception as e: logger.error(f"Error processing tool {tool_name}: {e}", exc_info=True) - # Tool omitted device-wide with no failed event -> manifest is incomplete. - scanned_manifest_complete = False + # Extraction failed device-wide but the tool was detected -> keep it for all users (no prune). + for u in all_users: + scanned_manifest.add((u, tool_name)) report_to_sentry(e, {**sentry_ctx, "phase": "process_tool", "tool_name": tool_name}, level="warning") logger.info("") @@ -2745,13 +2746,8 @@ def time_step(name: str, phase: str) -> Iterator[None]: # only the completed event carries manifest + covered users (backend prunes from them) logger.info("Sending scan completed event...") - # Fail closed: if any tool/user read errored without a scan_event=failed, the manifest may be - # missing a live tool -> send manifest=None so the backend treats this run as legacy (no prune). - if scanned_manifest_complete: - manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] - else: - manifest = None - logger.warning("Scan had read errors; sending manifest=None to skip pruning this run") + # manifest = (home_user, tool_name) detected present; backend set-diffs it to prune the rest. + manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", args.app_name, sentry_context=sentry_ctx, diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 1e37dd2..87238cf 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -4,11 +4,12 @@ set of enumerated home users, so the backend can set-diff and soft-delete (prune) tools that are no longer installed. -Correctness property under test (the load-bearing one): a tool whose read -ERRORED is NEVER recorded in the manifest, so a transient read failure can -never be mistaken for an uninstall. Tools that were sent successfully OR -short-circuited by the local hash-match dedup (still installed, just unchanged) -ARE recorded. +Correctness property under test (the load-bearing one): the manifest is built +from DETECTION/presence, not extraction success. A tool that was detected present +is recorded even if reading its config/rules errored, so a read failure can never +be mistaken for an uninstall (and never fail-closes the manifest to None). A tool +whose DETECTOR errored (presence unknown) is also kept. A tool is omitted ONLY +when its detector ran cleanly and did not find it (a real uninstall). Seams (mirroring the existing suite in test_send_and_persist.py / test_discovery_flow.py): @@ -257,17 +258,17 @@ def test_covered_home_users_matches_full_enumeration_not_manifest(self): ) -class TestManifestExcludesErroredReads(unittest.TestCase): - """The load-bearing property: a tool whose read raises is OMITTED from the - manifest, while a successfully-sent tool and a hash-match (unchanged, still - installed) tool are BOTH included. +class TestManifestFromPresence(unittest.TestCase): + """The load-bearing property: the manifest is built from DETECTION/presence, not + extraction success. A detected tool is recorded even if reading its config errored + (so a read failure is never mistaken for an uninstall and never nulls the manifest), + and a tool whose DETECTOR errored is kept too. A tool is omitted ONLY when its + detector ran cleanly and did not find it. - Seam: main() driven in-process with a mocked detector + discovery_cache and - a captured send_scan_event. This is necessary because the per-(tool, user) - loop body — with its success / hash-match / PermissionError branches — lives - inline inside main() and is not an independently callable unit. No - production code was changed to enable this; every name patched here is a - module-level import already present in ai_tools_discovery. + Seam: main() driven in-process with a mocked detector + discovery_cache and a + captured send_scan_event. The per-(tool, user) loop body lives inline inside main(), + so this is the only seam where the success / hash-match / read-error branches can be + forced deterministically. No production code was changed to enable this. """ def setUp(self): @@ -287,15 +288,16 @@ def _make_tool(name): # Distinct install_path per tool so the (name:path) dedup keeps all three. return {"name": name, "version": "1.0", "install_path": f"/opt/{name}", "projects": []} - def _run_main_capture_manifest(self, send_report_result=(True, False), filter_error=None): + def _run_main_capture_manifest(self, send_report_result=(True, False), filter_error=None, detector_failure=None): """Run main() with three crafted tools for one user: - ToolOK -> hash mismatch -> send path -> send result varies -> appended - ToolHashMatch -> hash match -> dedup short-circuit -> appended - ToolErr -> filter raises PermissionError -> NOT appended - send_report_result controls send_report_to_backend's (success, retryable) - return — pass (False, True) to exercise a read-success / upload-failure. - Returns the captured (manifest, covered_home_users) from the completed - send_scan_event call. + ToolOK -> hash mismatch -> send path + ToolHashMatch -> hash match -> dedup short-circuit + ToolErr -> filter raises (read/extraction error) + All three are DETECTED, so all three must appear in the manifest (presence-based). + send_report_result controls send_report_to_backend's (success, retryable). + detector_failure: if set, detect_all_tools reports that tool_name via its `failures` + set (a detector error) — it must also appear in the manifest though it isn't "found". + Returns the captured (manifest, covered_home_users) from the completed send_scan_event. """ adm = self.adm @@ -305,7 +307,13 @@ def _run_main_capture_manifest(self, send_report_result=(True, False), filter_er detector = Mock() detector.get_device_id.return_value = "dev-xyz" - detector.detect_all_tools.return_value = [tool_ok, tool_hm, tool_err] + + def _detect_all(user_home=None, failures=None): + # A detector error surfaces via the `failures` set (presence unknown -> kept in manifest). + if detector_failure and failures is not None: + failures.add(detector_failure) + return [tool_ok, tool_hm, tool_err] + detector.detect_all_tools.side_effect = _detect_all detector._set_canonical_vscode_copilot.return_value = None detector.process_single_tool.side_effect = lambda t: t @@ -366,43 +374,30 @@ def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=No return captured - def test_errored_tool_excluded_success_and_hashmatch_included(self): + def test_read_error_keeps_tool_in_manifest(self): + # A tool whose config read ERRORS is still detected present -> stays in the manifest (a read failure isn't an uninstall). captured = self._run_main_capture_manifest() self.assertIn("manifest", captured, "completed event was never sent") + self.assertIsNotNone(captured["manifest"], "a read error must NOT fail-close the manifest to None") pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} - # Success path recorded. - self.assertIn(("alice", "ToolOK"), pairs) - # Hash-match (unchanged, still installed) recorded. - self.assertIn(("alice", "ToolHashMatch"), pairs) - # Errored read is the load-bearing exclusion: a read failure must never - # look like an uninstall. - self.assertNotIn(("alice", "ToolErr"), pairs) - # Exactly the two non-errored pairs, nothing else. - self.assertEqual(len(captured["manifest"]), 2) - - def test_read_success_but_upload_failure_still_in_manifest(self): - # A tool whose READ succeeds but whose UPLOAD fails transiently - # (send_report_to_backend -> (False, retryable=True)) is still installed. - # It MUST stay in the manifest; otherwise the backend would mistake a - # transient upload failure for an uninstall and prune a live tool. This - # fails before the fix (ToolOK absent when its upload fails) and passes - # after (the manifest tracks what was SEEN, not what uploaded). + self.assertIn(("alice", "ToolOK"), pairs) # sent path + self.assertIn(("alice", "ToolHashMatch"), pairs) # hash-match (unchanged, still installed) + self.assertIn(("alice", "ToolErr"), pairs) # read errored but DETECTED -> kept + self.assertEqual(len(captured["manifest"]), 3) + + def test_upload_failure_keeps_tool_in_manifest(self): + # Presence is recorded before extraction, so a transient UPLOAD failure still keeps the tool in the manifest. captured = self._run_main_capture_manifest(send_report_result=(False, True)) self.assertIn("manifest", captured, "completed event was never sent") pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} - - # Read-success tool whose upload FAILED is still recorded. - self.assertIn(("alice", "ToolOK"), pairs) - # Hash-match (no upload at all) still recorded. - self.assertIn(("alice", "ToolHashMatch"), pairs) - # An errored READ remains excluded — that path is unchanged. - self.assertNotIn(("alice", "ToolErr"), pairs) - # Manifest is identical to the all-uploads-succeed case: read-success is - # what counts, not upload success. - self.assertEqual(len(captured["manifest"]), 2) + # All three detected tools present, regardless of upload outcome / read error. + self.assertEqual( + pairs, + {("alice", "ToolOK"), ("alice", "ToolHashMatch"), ("alice", "ToolErr")}, + ) def test_covered_home_users_includes_user_with_no_manifest_entry(self): # covered_home_users must come from the full enumeration (all_users), @@ -413,22 +408,29 @@ def test_covered_home_users_includes_user_with_no_manifest_entry(self): captured = self._run_main_capture_manifest() self.assertEqual(captured.get("covered_home_users"), ["alice"]) - def test_generic_read_error_fails_closed_manifest_none(self): - # A NON-Permission read error sends NO scan_event=failed, so the backend can't tell the - # scan was unclean. The completed event must fail closed: manifest=None (legacy = no prune), - # so a transient error can never drive a wrongful device-wide delete. (A PermissionError, by - # contrast, DOES send a failed event and keeps a real manifest — covered by the test above.) + def test_generic_read_error_does_not_fail_close(self): + # Regression: a generic read error used to fail-close the manifest to None (blocking all pruning); it must no longer. captured = self._run_main_capture_manifest( filter_error=RuntimeError("simulated generic read failure") ) self.assertIn("manifest", captured, "completed event was never sent") - self.assertIsNone( + self.assertIsNotNone( captured["manifest"], - "a generic (non-Permission) read error must fail closed -> manifest=None", + "a generic read error must NOT fail-close the manifest to None", ) - # covered_home_users is still reported (telemetry of who was enumerated). + pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} + self.assertIn(("alice", "ToolErr"), pairs) + self.assertEqual(len(captured["manifest"]), 3) self.assertEqual(captured.get("covered_home_users"), ["alice"]) + def test_detector_error_keeps_tool_in_manifest(self): + # A tool whose DETECTOR errors (presence unknown) is kept in the manifest. ToolGhost isn't "found" but its detector failed -> must appear. + captured = self._run_main_capture_manifest(detector_failure="ToolGhost") + pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} + self.assertIn(("alice", "ToolGhost"), pairs) + # The three detected tools are present too (ToolErr kept despite its read error). + self.assertEqual(len(captured["manifest"]), 4) + if __name__ == "__main__": unittest.main() From 83a19b37b58dcaac8f350bb83a432e8e26ce2c47 Mon Sep 17 00:00:00 2001 From: audit Date: Sat, 27 Jun 2026 11:22:18 +0530 Subject: [PATCH 06/10] fix(web-4679): per-user manifest + emission gate; mark scan unclean on detector error Addresses two Greptile P1s on the scan-manifest: - Phantom ownership: the manifest was added for every (user x globally-deduped tool), so a user-scoped tool one user has was attributed to co-resident users who never detected it -> backend could never prune their stale rows. Build the manifest from per-user detection, and only emit a report for a (user, tool) the user actually detected (gate on manifest membership). Add the missing Copilot CLI ownership discard (only Augment had it). - Umbrella names: a detector failure recorded detector.tool_name (e.g. 'GitHub Copilot'), not the concrete row names ('GitHub Copilot (VS Code)'), so the set-diff would prune the real surface rows. Instead mark the scan unclean (a 'failed' event before 'completed') so the backend skips pruning. Also add manifest_size + scan_incomplete to discovery metrics. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 50 +++++++-- tests/test_scan_completed_manifest.py | 105 +++++++++++++++--- 2 files changed, 131 insertions(+), 24 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index b70c498..e039ec5 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2807,6 +2807,8 @@ def _on_term_signal(signum, _frame) -> None: # (home_user, tool_name) detected present this run; backend set-diffs it in "completed" to prune the rest. scanned_manifest = set() + # Detection/extraction errors this run; if non-empty the scan is marked unclean so the backend skips pruning. + incomplete_reasons = [] # --- Drain pending reports from previous run --- with time_step("drain_pending_queue", "queue"): @@ -2906,9 +2908,16 @@ def _on_term_signal(signum, _frame) -> None: user_tools = detector.detect_all_tools( user_home=user_home, failures=user_detect_failures ) - # Detector errored -> presence unknown -> keep it in the manifest (don't treat as uninstalled). - for failed_tool_name in user_detect_failures: - scanned_manifest.add((user, failed_tool_name)) + # Record per-user presence at detection: a tool this user actually has stays in the + # manifest even if reading/uploading it later errors (a read failure isn't an + # uninstall), and only users who detected the tool get an entry (no phantom ownership). + for detected in user_tools: + scanned_manifest.add((user, detected.get('name', 'Unknown'))) + # A detector ERRORED -> presence unknown. detector.tool_name is an umbrella label + # (e.g. "GitHub Copilot"), not the concrete row name ("GitHub Copilot (VS Code)"), + # so it can't safely target the manifest. Skip pruning this run instead. + if user_detect_failures: + incomplete_reasons.append(f"detector error for user {user}") if user_tools: logger.info(f" Found {len(user_tools)} tool(s) for {user}:") @@ -2971,8 +2980,12 @@ def _on_term_signal(signum, _frame) -> None: user_home = Path.home() try: - # Record presence before extraction so a read error below can't drop a live tool. - scanned_manifest.add((user_name, tool_name)) + # Only report a tool for users who actually detected it (the manifest is + # the per-user presence set). all_tools is deduped globally, so without this + # a user-scoped tool one user has would otherwise be reported for every + # enumerated user — a phantom install the backend could never prune. + if (user_name, tool_name) not in scanned_manifest: + continue # Filter projects to only include this user's projects with time_step("filter_projects", "process"): @@ -2989,6 +3002,8 @@ def _on_term_signal(signum, _frame) -> None: f"{tool_filtered.get('_config_path') or tool_filtered.get('install_path')!r} " f"not owned by this user and no per-user data" ) + # Detected globally but not owned by this user -> drop the presence entry. + scanned_manifest.discard((user_name, tool_name)) continue # Ownership gate (Augment surfaces): same ~/.augment-keyed @@ -3001,7 +3016,7 @@ def _on_term_signal(signum, _frame) -> None: f"{tool_filtered.get('_config_path') or tool_filtered.get('install_path')!r} " f"not owned by this user and no per-user data" ) - # Not actually present for this user -> undo the optimistic presence record. + # Detected globally but not owned by this user -> drop the presence entry. scanned_manifest.discard((user_name, tool_name)) continue @@ -3214,9 +3229,8 @@ def _on_term_signal(signum, _frame) -> None: except Exception as e: logger.error(f"Error processing tool {tool_name}: {e}", exc_info=True) - # Extraction failed device-wide but the tool was detected -> keep it for all users (no prune). - for u in all_users: - scanned_manifest.add((u, tool_name)) + # Detected tools are already in the manifest from the detection phase, so a + # device-wide extraction failure here cannot drop a live tool (no re-add needed). report_to_sentry(e, {**sentry_ctx, "phase": "process_tool", "tool_name": tool_name}, level="warning") logger.info("") @@ -3253,6 +3267,8 @@ def _on_term_signal(signum, _frame) -> None: "os": platform.system(), "tool_count": len(tools), "user_count": len(all_users), + "manifest_size": len(scanned_manifest), + "scan_incomplete": bool(incomplete_reasons), "python_version": f"{sys.version_info.major}.{sys.version_info.minor}", "script_version": SCRIPT_VERSION, }, @@ -3264,6 +3280,22 @@ def _on_term_signal(signum, _frame) -> None: except Exception as metrics_err: logger.debug(f"Building/sending discovery metrics failed: {metrics_err}") + # Detection/extraction hit an error this run -> mark the scan unclean BEFORE the + # completed event so the backend's reconcile skips pruning (a missing tool may mean + # "couldn't read", not "uninstalled"). Sent first so scan_error is persisted before + # the completed event dispatches the reconcile. + if incomplete_reasons: + send_scan_event( + args.domain, args.api_key, device_id, run_id, "failed", + args.app_name, + scan_error={ + "error_type": "ScanIncomplete", + "message": "; ".join(incomplete_reasons[:20]), + "timestamp": datetime.utcnow().isoformat() + "Z", + }, + sentry_context=sentry_ctx, system_user=system_user, + ) + # only the completed event carries manifest + covered users (backend prunes from them) logger.info("Sending scan completed event...") # manifest = (home_user, tool_name) detected present; backend set-diffs it to prune the rest. diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 87238cf..23da1cc 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -5,11 +5,12 @@ (prune) tools that are no longer installed. Correctness property under test (the load-bearing one): the manifest is built -from DETECTION/presence, not extraction success. A tool that was detected present -is recorded even if reading its config/rules errored, so a read failure can never -be mistaken for an uninstall (and never fail-closes the manifest to None). A tool -whose DETECTOR errored (presence unknown) is also kept. A tool is omitted ONLY -when its detector ran cleanly and did not find it (a real uninstall). +from per-user DETECTION/presence, not extraction success. A tool that was detected +present is recorded even if reading its config/rules errored, so a read failure can +never be mistaken for an uninstall (and never fail-closes the manifest to None). +Only users who actually detected a tool get an entry (no phantom ownership). A tool +whose DETECTOR errored marks the scan unclean (the backend skips pruning) rather than +recording a name, since detector.tool_name is an umbrella label, not the concrete row. Seams (mirroring the existing suite in test_send_and_persist.py / test_discovery_flow.py): @@ -259,11 +260,11 @@ def test_covered_home_users_matches_full_enumeration_not_manifest(self): class TestManifestFromPresence(unittest.TestCase): - """The load-bearing property: the manifest is built from DETECTION/presence, not - extraction success. A detected tool is recorded even if reading its config errored - (so a read failure is never mistaken for an uninstall and never nulls the manifest), - and a tool whose DETECTOR errored is kept too. A tool is omitted ONLY when its - detector ran cleanly and did not find it. + """The load-bearing property: the manifest is built from per-user DETECTION/presence, + not extraction success. A detected tool is recorded even if reading its config errored + (so a read failure is never mistaken for an uninstall and never nulls the manifest); + only users who detected the tool get an entry; and a DETECTOR error marks the scan + unclean instead of recording an umbrella name. Seam: main() driven in-process with a mocked detector + discovery_cache and a captured send_scan_event. The per-(tool, user) loop body lives inline inside main(), @@ -352,6 +353,8 @@ def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=No if scan_event == "completed": captured["manifest"] = kw.get("manifest") captured["covered_home_users"] = kw.get("covered_home_users") + elif scan_event == "failed": + captured.setdefault("failed_events", []).append(kw.get("scan_error")) return (True, None) with patch.object(adm.platform, "system", return_value="Darwin"), \ @@ -423,13 +426,85 @@ def test_generic_read_error_does_not_fail_close(self): self.assertEqual(len(captured["manifest"]), 3) self.assertEqual(captured.get("covered_home_users"), ["alice"]) - def test_detector_error_keeps_tool_in_manifest(self): - # A tool whose DETECTOR errors (presence unknown) is kept in the manifest. ToolGhost isn't "found" but its detector failed -> must appear. + def test_detector_error_marks_scan_unclean(self): + # A DETECTOR error means presence is unknown, and detector.tool_name is only an umbrella + # label (e.g. "GitHub Copilot") that can't safely target the concrete install rows. So the + # run is marked unclean (a "failed" event) and the umbrella name is NOT added to the + # manifest -> the backend skips pruning rather than prune a real surface row. captured = self._run_main_capture_manifest(detector_failure="ToolGhost") pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} - self.assertIn(("alice", "ToolGhost"), pairs) - # The three detected tools are present too (ToolErr kept despite its read error). - self.assertEqual(len(captured["manifest"]), 4) + self.assertNotIn(("alice", "ToolGhost"), pairs) + # Only the three actually-detected tools remain. + self.assertEqual(len(captured["manifest"]), 3) + self.assertTrue( + captured.get("failed_events"), + "a detector error must mark the scan unclean so the backend skips pruning this run", + ) + + def test_per_user_detection_no_phantom_ownership(self): + # Phantom-ownership regression: all_tools is deduped globally, so a user-scoped tool one + # user has must NOT be attributed to a co-resident user who did not detect it. Alice has + # ToolA, Bob has ToolB; the manifest must contain exactly each user's own tool. + adm = self.adm + tool_a = self._make_tool("ToolA") + tool_b = self._make_tool("ToolB") + + detector = Mock() + detector.get_device_id.return_value = "dev-xyz" + + def _detect_all(user_home=None, failures=None): + home = str(user_home or "") + if home.endswith("alice"): + return [tool_a] + if home.endswith("bob"): + return [tool_b] + return [] + detector.detect_all_tools.side_effect = _detect_all + detector._set_canonical_vscode_copilot.return_value = None + detector._set_canonical_augment_surface.return_value = None + detector.process_single_tool.side_effect = lambda t: t + detector.filter_tool_projects_by_user.side_effect = lambda t, _h: t + detector.generate_single_tool_report.side_effect = ( + lambda tool, device_id, home_user, system_user=None, run_id=None: {"tools": [tool]} + ) + + dc = Mock() + dc.acquire_lock.return_value = "acquired" + dc.heartbeat_start.return_value = Mock() + dc.get_cached_hash.return_value = None + dc.update_tool.return_value = None + dc.UNBOUND_DIR = "/tmp/unbound-test" + dc.last_lock_error = None + + captured = {} + + def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=None, **kw): + if scan_event == "completed": + captured["manifest"] = kw.get("manifest") + return (True, None) + + with patch.object(adm.platform, "system", return_value="Darwin"), \ + patch.object(adm, "AIToolsDetector", return_value=detector), \ + patch.object(adm, "discovery_cache", dc), \ + patch.object(adm, "get_all_users_macos", return_value=["alice", "bob"]), \ + patch.object(adm, "compute_payload_hash", side_effect=lambda t: "h-" + t["name"]), \ + patch.object(adm, "send_report_to_backend", return_value=(True, False)), \ + patch.object(adm, "send_scan_event", side_effect=_send_scan_event), \ + patch.object(adm, "send_discovery_metrics", Mock()), \ + patch.object(adm, "load_pending_reports", return_value=[]), \ + patch.object(adm, "save_failed_reports", Mock()), \ + patch.object(adm, "report_to_sentry", Mock()), \ + patch.object(utils_mod, "_SENTRY_DSN", ""), \ + patch.object(sys, "argv", self.argv): + try: + adm.main() + except SystemExit: + pass + + pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} + self.assertEqual(pairs, {("alice", "ToolA"), ("bob", "ToolB")}) + self.assertNotIn(("bob", "ToolA"), pairs) + self.assertNotIn(("alice", "ToolB"), pairs) if __name__ == "__main__": From a62873de9009618b0c3a9d242279c8dbcd920eee Mon Sep 17 00:00:00 2001 From: audit Date: Sat, 27 Jun 2026 19:38:47 +0530 Subject: [PATCH 07/10] test(web-4679): lock JetBrains prune-key naming determinism Regression guard for the discovery prune key: the JetBrains tool name must stay the bare display_name (version + license/plan kept in separate fields), so a version bump or Free<->Licensed change never orphans the install row against the manifest. Fails CI if a future change re-embeds version/plan into the name. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_jetbrains_naming_determinism.py | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 tests/test_jetbrains_naming_determinism.py diff --git a/tests/test_jetbrains_naming_determinism.py b/tests/test_jetbrains_naming_determinism.py new file mode 100644 index 0000000..a8ac874 --- /dev/null +++ b/tests/test_jetbrains_naming_determinism.py @@ -0,0 +1,62 @@ +"""WEB-4679 prune-key safety: the JetBrains tool *name* (which the backend uses as the +prune key, matched exactly against the scan manifest) must exclude version and license/plan. + +If a future change re-embeds version/plan into the name (e.g. reverting to +f"{display_name} {version} ({plan})"), every JetBrains install row would orphan against the +manifest on the next scan and be wrongly pruned. These tests fail CI if that regression lands. +""" +import logging +import unittest +from unittest.mock import patch + +from scripts.coding_discovery_tools.macos.jetbrains.jetbrains import MacOSJetBrainsDetector + +logging.disable(logging.CRITICAL) + + +class TestJetBrainsNamingDeterminism(unittest.TestCase): + def setUp(self): + self.det = MacOSJetBrainsDetector() + + def test_display_name_is_version_free_and_stable_across_bumps(self): + # A patch/minor version bump renames the folder but must NOT change display_name, + # so the prune key stays invariant across upgrades. + for folder in ("PyCharm2025.3", "PyCharm2025.3.1", "PyCharm2026.1"): + name, version = self.det._parse_ide_name_and_version(folder) + self.assertEqual(name, "PyCharm", f"{folder} must map to stable 'PyCharm'") + self.assertNotIn(version, name, "version must not leak into the display name") + self.assertEqual( + self.det._parse_ide_name_and_version("IntelliJIdea2025.3")[0], "IntelliJ IDEA" + ) + + def test_mapping_values_carry_no_version_or_plan(self): + for _prefix, name in MacOSJetBrainsDetector.IDE_NAME_MAPPING.items(): + self.assertNotRegex(name, r"\d", f"{name!r} must not embed a version digit") + self.assertNotIn("(", name, f"{name!r} must not embed a (plan) suffix") + + def test_detected_tool_name_excludes_version_and_plan(self): + # Lock the prune-key invariant: detect() sets name = display_name ONLY, keeping + # version and plan in separate fields (never concatenated into the name). + fake_ide = { + "display_name": "PyCharm", + "version": "2025.3.1", + "plan": "Licensed", + "config_path": "/nonexistent/pycharm", + "folder_name": "PyCharm2025.3.1", + } + with patch.object(self.det, "_scan_for_ides", return_value=[fake_ide]), \ + patch.object(self.det, "_get_plugins", return_value=[]): + tools = self.det.detect() + + self.assertEqual(len(tools), 1) + tool = tools[0] + self.assertEqual(tool["name"], "PyCharm", "prune key (name) must be the bare display_name") + self.assertNotIn("2025", tool["name"]) + self.assertNotIn("Licensed", tool["name"]) + # version + plan are preserved, just not in the name (so they can't move the key). + self.assertEqual(tool["version"], "2025.3.1") + self.assertEqual(tool["plan"], "Licensed") + + +if __name__ == "__main__": + unittest.main() From a7799b7dbba827e0cc22bf4d64ba0a85edcb6827 Mon Sep 17 00:00:00 2001 From: audit Date: Sat, 27 Jun 2026 20:20:11 +0530 Subject: [PATCH 08/10] fix(web-4679): per-user extension scoping + atomic incomplete-scan signal Addresses the re-review on the prior commit: - Phantom ownership (Greptile P1): Roo/Cline/Kilo detect() now scope to the per-user home set by detect_tool_for_user, so a root multi-user scan no longer self-enumerates /Users and attributes every user's extension to the outer-loop user. - Incomplete-scan false-delete (Cursor + Greptile P1): an incomplete scan now sends NO manifest on the completed event itself (atomic), instead of a separate 'failed' event whose loss could leave the backend pruning a partial inventory. Removed the fragile separate event. - Trimmed verbose comments; folded the standalone JetBrains determinism test into test_scan_completed_manifest.py. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 47 +++++--------- .../macos/cline/cline.py | 6 ++ .../macos/kilocode/kilocode.py | 8 ++- .../macos/roo_code/roo_code.py | 6 ++ tests/test_jetbrains_naming_determinism.py | 62 ------------------- tests/test_scan_completed_manifest.py | 62 ++++++++++++++----- 6 files changed, 82 insertions(+), 109 deletions(-) delete mode 100644 tests/test_jetbrains_naming_determinism.py diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index e039ec5..60fc7d9 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -2807,7 +2807,7 @@ def _on_term_signal(signum, _frame) -> None: # (home_user, tool_name) detected present this run; backend set-diffs it in "completed" to prune the rest. scanned_manifest = set() - # Detection/extraction errors this run; if non-empty the scan is marked unclean so the backend skips pruning. + # Detector errors this run; if non-empty, no manifest is sent so the backend won't prune. incomplete_reasons = [] # --- Drain pending reports from previous run --- @@ -2908,14 +2908,12 @@ def _on_term_signal(signum, _frame) -> None: user_tools = detector.detect_all_tools( user_home=user_home, failures=user_detect_failures ) - # Record per-user presence at detection: a tool this user actually has stays in the - # manifest even if reading/uploading it later errors (a read failure isn't an - # uninstall), and only users who detected the tool get an entry (no phantom ownership). + # Per-user presence: a detected tool stays in the manifest even if reading it later + # errors (a read failure isn't an uninstall). for detected in user_tools: scanned_manifest.add((user, detected.get('name', 'Unknown'))) - # A detector ERRORED -> presence unknown. detector.tool_name is an umbrella label - # (e.g. "GitHub Copilot"), not the concrete row name ("GitHub Copilot (VS Code)"), - # so it can't safely target the manifest. Skip pruning this run instead. + # A detector error means presence is unknown for this user -> mark the scan incomplete + # so it doesn't prune (detector.tool_name is an umbrella label, not the real row name). if user_detect_failures: incomplete_reasons.append(f"detector error for user {user}") @@ -2980,10 +2978,8 @@ def _on_term_signal(signum, _frame) -> None: user_home = Path.home() try: - # Only report a tool for users who actually detected it (the manifest is - # the per-user presence set). all_tools is deduped globally, so without this - # a user-scoped tool one user has would otherwise be reported for every - # enumerated user — a phantom install the backend could never prune. + # all_tools is deduped globally; only report a tool for users who actually + # detected it (i.e. it's in their manifest) to avoid phantom installs. if (user_name, tool_name) not in scanned_manifest: continue @@ -3229,8 +3225,8 @@ def _on_term_signal(signum, _frame) -> None: except Exception as e: logger.error(f"Error processing tool {tool_name}: {e}", exc_info=True) - # Detected tools are already in the manifest from the detection phase, so a - # device-wide extraction failure here cannot drop a live tool (no re-add needed). + # Detected tools are already in the manifest from the detection phase, so this + # extraction failure can't drop a live tool. report_to_sentry(e, {**sentry_ctx, "phase": "process_tool", "tool_name": tool_name}, level="warning") logger.info("") @@ -3280,26 +3276,13 @@ def _on_term_signal(signum, _frame) -> None: except Exception as metrics_err: logger.debug(f"Building/sending discovery metrics failed: {metrics_err}") - # Detection/extraction hit an error this run -> mark the scan unclean BEFORE the - # completed event so the backend's reconcile skips pruning (a missing tool may mean - # "couldn't read", not "uninstalled"). Sent first so scan_error is persisted before - # the completed event dispatches the reconcile. - if incomplete_reasons: - send_scan_event( - args.domain, args.api_key, device_id, run_id, "failed", - args.app_name, - scan_error={ - "error_type": "ScanIncomplete", - "message": "; ".join(incomplete_reasons[:20]), - "timestamp": datetime.utcnow().isoformat() + "Z", - }, - sentry_context=sentry_ctx, system_user=system_user, - ) - - # only the completed event carries manifest + covered users (backend prunes from them) logger.info("Sending scan completed event...") - # manifest = (home_user, tool_name) detected present; backend set-diffs it to prune the rest. - manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] + # An incomplete scan sends no manifest so the backend won't prune from a partial inventory. + # Carrying this on the completed event itself (vs a separate signal) keeps it atomic. + if incomplete_reasons: + manifest = None + else: + manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", args.app_name, sentry_context=sentry_ctx, system_user=system_user, diff --git a/scripts/coding_discovery_tools/macos/cline/cline.py b/scripts/coding_discovery_tools/macos/cline/cline.py index d965585..a0adfaa 100644 --- a/scripts/coding_discovery_tools/macos/cline/cline.py +++ b/scripts/coding_discovery_tools/macos/cline/cline.py @@ -76,6 +76,12 @@ def detect(self) -> Optional[List[Dict]]: List of dicts containing tool info for each IDE with Cline installed, or None if not found in any IDE """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only. Without + # this, a root scan self-enumerates /Users and attributes every user's extension to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_cline_for_user(Path(scoped_home)) or None + all_results = [] if is_running_as_root(): diff --git a/scripts/coding_discovery_tools/macos/kilocode/kilocode.py b/scripts/coding_discovery_tools/macos/kilocode/kilocode.py index 196b7d9..ae93c4f 100644 --- a/scripts/coding_discovery_tools/macos/kilocode/kilocode.py +++ b/scripts/coding_discovery_tools/macos/kilocode/kilocode.py @@ -50,6 +50,12 @@ def detect(self) -> Optional[Dict]: Returns: Dict containing tool info (name, version, install_path) or None if not found """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only. Without + # this, a root scan self-enumerates /Users and attributes every user's extension to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._check_user_for_kilocode(Path(scoped_home)) + # When running as root, scan all user directories first if is_running_as_root(): user_kilocode_info = scan_user_directories( @@ -57,7 +63,7 @@ def detect(self) -> Optional[Dict]: ) if user_kilocode_info: return user_kilocode_info - + # Check current user (works for both root and regular users) return self._check_user_for_kilocode(Path.home()) diff --git a/scripts/coding_discovery_tools/macos/roo_code/roo_code.py b/scripts/coding_discovery_tools/macos/roo_code/roo_code.py index 29638eb..f8fa7d1 100644 --- a/scripts/coding_discovery_tools/macos/roo_code/roo_code.py +++ b/scripts/coding_discovery_tools/macos/roo_code/roo_code.py @@ -78,6 +78,12 @@ def detect(self) -> Optional[List[Dict]]: List of dicts containing tool info for each IDE with Roo Code installed, or None if not found in any IDE """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only. Without + # this, a root scan self-enumerates /Users and attributes every user's extension to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_roo_for_user(Path(scoped_home)) or None + all_results = [] if is_running_as_root(): diff --git a/tests/test_jetbrains_naming_determinism.py b/tests/test_jetbrains_naming_determinism.py deleted file mode 100644 index a8ac874..0000000 --- a/tests/test_jetbrains_naming_determinism.py +++ /dev/null @@ -1,62 +0,0 @@ -"""WEB-4679 prune-key safety: the JetBrains tool *name* (which the backend uses as the -prune key, matched exactly against the scan manifest) must exclude version and license/plan. - -If a future change re-embeds version/plan into the name (e.g. reverting to -f"{display_name} {version} ({plan})"), every JetBrains install row would orphan against the -manifest on the next scan and be wrongly pruned. These tests fail CI if that regression lands. -""" -import logging -import unittest -from unittest.mock import patch - -from scripts.coding_discovery_tools.macos.jetbrains.jetbrains import MacOSJetBrainsDetector - -logging.disable(logging.CRITICAL) - - -class TestJetBrainsNamingDeterminism(unittest.TestCase): - def setUp(self): - self.det = MacOSJetBrainsDetector() - - def test_display_name_is_version_free_and_stable_across_bumps(self): - # A patch/minor version bump renames the folder but must NOT change display_name, - # so the prune key stays invariant across upgrades. - for folder in ("PyCharm2025.3", "PyCharm2025.3.1", "PyCharm2026.1"): - name, version = self.det._parse_ide_name_and_version(folder) - self.assertEqual(name, "PyCharm", f"{folder} must map to stable 'PyCharm'") - self.assertNotIn(version, name, "version must not leak into the display name") - self.assertEqual( - self.det._parse_ide_name_and_version("IntelliJIdea2025.3")[0], "IntelliJ IDEA" - ) - - def test_mapping_values_carry_no_version_or_plan(self): - for _prefix, name in MacOSJetBrainsDetector.IDE_NAME_MAPPING.items(): - self.assertNotRegex(name, r"\d", f"{name!r} must not embed a version digit") - self.assertNotIn("(", name, f"{name!r} must not embed a (plan) suffix") - - def test_detected_tool_name_excludes_version_and_plan(self): - # Lock the prune-key invariant: detect() sets name = display_name ONLY, keeping - # version and plan in separate fields (never concatenated into the name). - fake_ide = { - "display_name": "PyCharm", - "version": "2025.3.1", - "plan": "Licensed", - "config_path": "/nonexistent/pycharm", - "folder_name": "PyCharm2025.3.1", - } - with patch.object(self.det, "_scan_for_ides", return_value=[fake_ide]), \ - patch.object(self.det, "_get_plugins", return_value=[]): - tools = self.det.detect() - - self.assertEqual(len(tools), 1) - tool = tools[0] - self.assertEqual(tool["name"], "PyCharm", "prune key (name) must be the bare display_name") - self.assertNotIn("2025", tool["name"]) - self.assertNotIn("Licensed", tool["name"]) - # version + plan are preserved, just not in the name (so they can't move the key). - self.assertEqual(tool["version"], "2025.3.1") - self.assertEqual(tool["plan"], "Licensed") - - -if __name__ == "__main__": - unittest.main() diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 23da1cc..d78c75a 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -44,6 +44,7 @@ import scripts.coding_discovery_tools.utils as utils_mod from scripts.coding_discovery_tools.utils import send_scan_event +from scripts.coding_discovery_tools.macos.jetbrains.jetbrains import MacOSJetBrainsDetector REPO_ROOT = Path(__file__).resolve().parent.parent @@ -263,8 +264,8 @@ class TestManifestFromPresence(unittest.TestCase): """The load-bearing property: the manifest is built from per-user DETECTION/presence, not extraction success. A detected tool is recorded even if reading its config errored (so a read failure is never mistaken for an uninstall and never nulls the manifest); - only users who detected the tool get an entry; and a DETECTOR error marks the scan - unclean instead of recording an umbrella name. + only users who detected the tool get an entry; and a DETECTOR error sends no manifest at + all (so the backend skips pruning) rather than recording an umbrella name. Seam: main() driven in-process with a mocked detector + discovery_cache and a captured send_scan_event. The per-(tool, user) loop body lives inline inside main(), @@ -426,19 +427,13 @@ def test_generic_read_error_does_not_fail_close(self): self.assertEqual(len(captured["manifest"]), 3) self.assertEqual(captured.get("covered_home_users"), ["alice"]) - def test_detector_error_marks_scan_unclean(self): - # A DETECTOR error means presence is unknown, and detector.tool_name is only an umbrella - # label (e.g. "GitHub Copilot") that can't safely target the concrete install rows. So the - # run is marked unclean (a "failed" event) and the umbrella name is NOT added to the - # manifest -> the backend skips pruning rather than prune a real surface row. + def test_detector_error_sends_no_manifest(self): + # A detector error means presence is unknown this run, so NO manifest is sent — atomically + # on the completed event — and the backend (seeing no manifest) skips pruning entirely. captured = self._run_main_capture_manifest(detector_failure="ToolGhost") - pairs = {(e["home_user"], e["tool_name"]) for e in captured["manifest"]} - self.assertNotIn(("alice", "ToolGhost"), pairs) - # Only the three actually-detected tools remain. - self.assertEqual(len(captured["manifest"]), 3) - self.assertTrue( - captured.get("failed_events"), - "a detector error must mark the scan unclean so the backend skips pruning this run", + self.assertIsNone( + captured["manifest"], + "a detector error must send no manifest so the backend can't prune from a partial scan", ) def test_per_user_detection_no_phantom_ownership(self): @@ -507,5 +502,44 @@ def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=No self.assertNotIn(("alice", "ToolB"), pairs) +class TestJetBrainsNamingDeterminism(unittest.TestCase): + """The JetBrains tool name is the backend prune key (matched exactly vs the manifest), so it + must exclude version and license/plan — otherwise a version bump or Free<->Licensed change + would orphan the install row and wrongly prune it. Fails if a change re-embeds them in the name. + """ + + def setUp(self): + self.det = MacOSJetBrainsDetector() + + def test_display_name_is_version_free_and_stable_across_bumps(self): + for folder in ("PyCharm2025.3", "PyCharm2025.3.1", "PyCharm2026.1"): + name, version = self.det._parse_ide_name_and_version(folder) + self.assertEqual(name, "PyCharm", f"{folder} must map to stable 'PyCharm'") + self.assertNotIn(version, name, "version must not leak into the display name") + self.assertEqual( + self.det._parse_ide_name_and_version("IntelliJIdea2025.3")[0], "IntelliJ IDEA" + ) + + def test_mapping_values_carry_no_version_or_plan(self): + for _prefix, name in MacOSJetBrainsDetector.IDE_NAME_MAPPING.items(): + self.assertNotRegex(name, r"\d", f"{name!r} must not embed a version digit") + self.assertNotIn("(", name, f"{name!r} must not embed a (plan) suffix") + + def test_detected_tool_name_excludes_version_and_plan(self): + # detect() sets name = display_name ONLY; version and plan stay in separate fields. + fake_ide = { + "display_name": "PyCharm", "version": "2025.3.1", "plan": "Licensed", + "config_path": "/nonexistent/pycharm", "folder_name": "PyCharm2025.3.1", + } + with patch.object(self.det, "_scan_for_ides", return_value=[fake_ide]), \ + patch.object(self.det, "_get_plugins", return_value=[]): + tools = self.det.detect() + self.assertEqual(tools[0]["name"], "PyCharm", "prune key (name) must be the bare display_name") + self.assertNotIn("2025", tools[0]["name"]) + self.assertNotIn("Licensed", tools[0]["name"]) + self.assertEqual(tools[0]["version"], "2025.3.1") + self.assertEqual(tools[0]["plan"], "Licensed") + + if __name__ == "__main__": unittest.main() From 75ac37473d6094e04ae910fd3341f99bf85271a8 Mon Sep 17 00:00:00 2001 From: audit Date: Sat, 27 Jun 2026 20:34:11 +0530 Subject: [PATCH 09/10] chore(web-4679): trim verbose docstrings/comments in manifest tests Comment-only: concise module/class/test docstrings; fix stale references; no test logic change. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_scan_completed_manifest.py | 96 +++++++-------------------- 1 file changed, 24 insertions(+), 72 deletions(-) diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index d78c75a..3f5f2c6 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -1,34 +1,13 @@ -""" -Tests for WEB-4679: the scan "completed" event carries a manifest of the -(home_user, tool_name) pairs that were actually read this run, plus the full -set of enumerated home users, so the backend can set-diff and soft-delete -(prune) tools that are no longer installed. - -Correctness property under test (the load-bearing one): the manifest is built -from per-user DETECTION/presence, not extraction success. A tool that was detected -present is recorded even if reading its config/rules errored, so a read failure can -never be mistaken for an uninstall (and never fail-closes the manifest to None). -Only users who actually detected a tool get an entry (no phantom ownership). A tool -whose DETECTOR errored marks the scan unclean (the backend skips pruning) rather than -recording a name, since detector.tool_name is an umbrella label, not the concrete row. - -Seams (mirroring the existing suite in test_send_and_persist.py / -test_discovery_flow.py): - * TestSendScanEventManifest -> utils.send_scan_event against a real - localhost HTTP server (records POST bodies). Covers the payload-shaping - contract + backward compatibility. - * TestCompletedEventManifestCLI -> main() via subprocess against a real - localhost HTTP server. Covers the end-to-end completed-event payload and - that in_progress/failed events do NOT carry a manifest. - * TestManifestExcludesErroredReads -> main() driven IN-PROCESS with a mock - detector so a per-tool read error / hash-match / success can be forced - deterministically. This is the only seam where the errored-read branch can - be isolated, because the per-(tool, user) loop body lives inline in main(). - -Only external environment is mocked: HTTP backend (real server on localhost), -HOME (so the subprocess gets an isolated discovery lock/cache and never exits -early on a live lock from another run), _SENTRY_DSN (no real Sentry calls), -discovery_cache / detector (in-process seam only). No network is required. +"""WEB-4679: the "completed" scan event carries a manifest of detected (home_user, tool_name) +pairs + the covered home users, so the backend can set-diff and prune what's gone. + +Properties: the manifest is built from per-user DETECTION (not extraction success), so a read +error keeps a detected tool; only users who detected a tool get an entry (no phantom ownership); +a DETECTOR error sends no manifest (backend then skips pruning). + +Seams: TestSendScanEventManifest (send_scan_event vs a localhost server), TestCompletedEventManifestCLI +(main() via subprocess), TestManifestFromPresence (main() in-process with a mocked detector), +TestJetBrainsNamingDeterminism (prune-key naming). Only HTTP/HOME/_SENTRY_DSN/discovery_cache are mocked. """ import json @@ -137,10 +116,8 @@ def test_legacy_call_omits_both_keys(self, _sleep): @patch("time.sleep") @patch.object(utils_mod, "_SENTRY_DSN", "") def test_empty_manifest_still_sent(self, _sleep): - # An empty manifest is meaningfully different from "no manifest": it - # tells the backend "this scope had zero readable tools" (prune-all - # within scope). It must be sent (key present), since the production - # guard is `is not None`, not truthiness. + # Empty manifest != "no manifest": it means "zero tools in scope" and must be sent + # (key present), since the backend guard is `is not None`, not truthiness. success, _retryable = send_scan_event( self.base_url, "test-key", @@ -165,9 +142,7 @@ class TestCompletedEventManifestCLI(_ServerTestCase): def _run_cli(self, timeout=600): env = os.environ.copy() - # Isolate the discovery state dir (lock + cache) under a throwaway HOME - # so the run never exits early on a live lock left by another process, - # and starts from a cold cache (deterministic hash-match behavior). + # Throwaway HOME: isolated lock/cache so the run isn't blocked by a live lock and starts cold. env["HOME"] = tempfile.mkdtemp(prefix="web4679_home_") return subprocess.run( [ @@ -235,13 +210,8 @@ def test_non_completed_events_have_no_manifest(self): ) def test_covered_home_users_matches_full_enumeration_not_manifest(self): - # covered_home_users is sourced from the full user enumeration - # (all_users), NOT only from users that produced manifest entries. So a - # user who contributed zero manifest entries still appears in - # covered_home_users. We assert this invariant without needing to force - # a specific zero-tool user on the host: every home_user that appears in - # the manifest must also appear in covered_home_users, and - # covered_home_users must be a superset of the manifest's user set. + # covered_home_users comes from the full enumeration, not the manifest's users — so it must + # be a superset of the manifest's user set (asserted without forcing a zero-tool user). result = self._run_cli() self.assertEqual(result.returncode, 0, f"stderr: {result.stderr[-2000:]}") @@ -261,17 +231,9 @@ def test_covered_home_users_matches_full_enumeration_not_manifest(self): class TestManifestFromPresence(unittest.TestCase): - """The load-bearing property: the manifest is built from per-user DETECTION/presence, - not extraction success. A detected tool is recorded even if reading its config errored - (so a read failure is never mistaken for an uninstall and never nulls the manifest); - only users who detected the tool get an entry; and a DETECTOR error sends no manifest at - all (so the backend skips pruning) rather than recording an umbrella name. - - Seam: main() driven in-process with a mocked detector + discovery_cache and a - captured send_scan_event. The per-(tool, user) loop body lives inline inside main(), - so this is the only seam where the success / hash-match / read-error branches can be - forced deterministically. No production code was changed to enable this. - """ + """Manifest is built from per-user DETECTION: a read error keeps a detected tool; only users who + detected a tool get an entry; a DETECTOR error sends no manifest. Driven via main() in-process + with a mocked detector + captured send_scan_event.""" def setUp(self): import scripts.coding_discovery_tools.ai_tools_discovery as adm @@ -291,16 +253,9 @@ def _make_tool(name): return {"name": name, "version": "1.0", "install_path": f"/opt/{name}", "projects": []} def _run_main_capture_manifest(self, send_report_result=(True, False), filter_error=None, detector_failure=None): - """Run main() with three crafted tools for one user: - ToolOK -> hash mismatch -> send path - ToolHashMatch -> hash match -> dedup short-circuit - ToolErr -> filter raises (read/extraction error) - All three are DETECTED, so all three must appear in the manifest (presence-based). - send_report_result controls send_report_to_backend's (success, retryable). - detector_failure: if set, detect_all_tools reports that tool_name via its `failures` - set (a detector error) — it must also appear in the manifest though it isn't "found". - Returns the captured (manifest, covered_home_users) from the completed send_scan_event. - """ + """Run main() with three detected tools for one user: ToolOK (send), ToolHashMatch (dedup + skip), ToolErr (filter raises). detector_failure, if set, makes detect_all_tools report a + detector error. Returns the captured (manifest, covered_home_users) from the completed event.""" adm = self.adm tool_ok = self._make_tool("ToolOK") @@ -311,7 +266,7 @@ def _run_main_capture_manifest(self, send_report_result=(True, False), filter_er detector.get_device_id.return_value = "dev-xyz" def _detect_all(user_home=None, failures=None): - # A detector error surfaces via the `failures` set (presence unknown -> kept in manifest). + # A detector error surfaces via the `failures` set (-> scan marked incomplete). if detector_failure and failures is not None: failures.add(detector_failure) return [tool_ok, tool_hm, tool_err] @@ -404,11 +359,8 @@ def test_upload_failure_keeps_tool_in_manifest(self): ) def test_covered_home_users_includes_user_with_no_manifest_entry(self): - # covered_home_users must come from the full enumeration (all_users), - # so even though "alice" is the only user and one of her tools errored, - # she still appears. More importantly, this proves covered_home_users is - # not derived from the manifest: a user whose every tool errored would - # still be covered (bounding the prune scope correctly). + # covered_home_users comes from the full enumeration, not the manifest, so a user whose + # tools all errored is still covered (bounds the prune scope correctly). captured = self._run_main_capture_manifest() self.assertEqual(captured.get("covered_home_users"), ["alice"]) From 05f400c02393771d83aa91c7ba0040733089873b Mon Sep 17 00:00:00 2001 From: audit Date: Sat, 27 Jun 2026 23:30:50 +0530 Subject: [PATCH 10/10] fix(web-4679): scope Linux/Windows extension detectors per-user; omit covered scope on incomplete scan Greptile re-review residuals (both P1, on the latest head): - Phantom ownership: the per-user scoping fix was macOS-only. Linux + Windows Cline/Kilo/Roo detect() now also scope to the per-user home set by detect_tool_for_user, so an elevated scan can't attribute one user's extension to another. - Incomplete scope: on a detector failure the completed event now omits covered_home_users too (not just manifest), so the backend never sees a prune scope without an inventory. Tests: extension-detector-scopes-to-user-home + detector-error-omits-covered. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai_tools_discovery.py | 9 ++++--- .../linux/cline/cline.py | 5 ++++ .../linux/kilocode/kilocode.py | 5 ++++ .../linux/roo_code/roo_code.py | 5 ++++ .../windows/cline/cline.py | 6 +++++ .../windows/kilocode/kilocode.py | 8 +++++- .../windows/roo_code/roo_code.py | 6 +++++ tests/test_scan_completed_manifest.py | 25 ++++++++++++++----- 8 files changed, 58 insertions(+), 11 deletions(-) diff --git a/scripts/coding_discovery_tools/ai_tools_discovery.py b/scripts/coding_discovery_tools/ai_tools_discovery.py index 60fc7d9..d6fb4e1 100644 --- a/scripts/coding_discovery_tools/ai_tools_discovery.py +++ b/scripts/coding_discovery_tools/ai_tools_discovery.py @@ -3277,16 +3277,17 @@ def _on_term_signal(signum, _frame) -> None: logger.debug(f"Building/sending discovery metrics failed: {metrics_err}") logger.info("Sending scan completed event...") - # An incomplete scan sends no manifest so the backend won't prune from a partial inventory. - # Carrying this on the completed event itself (vs a separate signal) keeps it atomic. + # An incomplete scan sends neither manifest nor covered scope, so the backend has no + # partial inventory to prune from (atomic on this event — no separate signal to lose). if incomplete_reasons: - manifest = None + manifest, covered = None, None else: manifest = [{"home_user": hu, "tool_name": tn} for hu, tn in sorted(scanned_manifest)] + covered = all_users success, _ = send_scan_event( args.domain, args.api_key, device_id, run_id, "completed", args.app_name, sentry_context=sentry_ctx, system_user=system_user, - manifest=manifest, covered_home_users=all_users, + manifest=manifest, covered_home_users=covered, ) if success: logger.info("✓ Scan completed event sent successfully") diff --git a/scripts/coding_discovery_tools/linux/cline/cline.py b/scripts/coding_discovery_tools/linux/cline/cline.py index 2a7da94..83a8d0d 100644 --- a/scripts/coding_discovery_tools/linux/cline/cline.py +++ b/scripts/coding_discovery_tools/linux/cline/cline.py @@ -38,6 +38,11 @@ def tool_name(self) -> str: return "Cline" def detect(self) -> Optional[List[Dict]]: + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_cline_for_user(Path(scoped_home)) or None all_results = [] for user_home in get_linux_user_homes(): try: diff --git a/scripts/coding_discovery_tools/linux/kilocode/kilocode.py b/scripts/coding_discovery_tools/linux/kilocode/kilocode.py index 075d50d..3756ecb 100644 --- a/scripts/coding_discovery_tools/linux/kilocode/kilocode.py +++ b/scripts/coding_discovery_tools/linux/kilocode/kilocode.py @@ -33,6 +33,11 @@ def tool_name(self) -> str: return "Kilo Code" def detect(self) -> Optional[Dict]: + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._check_user_for_kilocode(Path(scoped_home)) for user_home in get_linux_user_homes(): result = self._check_user_for_kilocode(user_home) if result: diff --git a/scripts/coding_discovery_tools/linux/roo_code/roo_code.py b/scripts/coding_discovery_tools/linux/roo_code/roo_code.py index 7f5f2cb..2e712ac 100644 --- a/scripts/coding_discovery_tools/linux/roo_code/roo_code.py +++ b/scripts/coding_discovery_tools/linux/roo_code/roo_code.py @@ -39,6 +39,11 @@ def tool_name(self) -> str: return "Roo Code" def detect(self) -> Optional[List[Dict]]: + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_roo_for_user(Path(scoped_home)) or None all_results = [] for user_home in get_linux_user_homes(): try: diff --git a/scripts/coding_discovery_tools/windows/cline/cline.py b/scripts/coding_discovery_tools/windows/cline/cline.py index 1c987a2..3900806 100644 --- a/scripts/coding_discovery_tools/windows/cline/cline.py +++ b/scripts/coding_discovery_tools/windows/cline/cline.py @@ -66,6 +66,12 @@ def detect(self) -> Optional[List[Dict]]: List of dicts containing tool info for each IDE with Cline installed, or None if not found in any IDE """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_cline_for_user(Path(scoped_home)) or None + all_results = [] if is_running_as_admin(): diff --git a/scripts/coding_discovery_tools/windows/kilocode/kilocode.py b/scripts/coding_discovery_tools/windows/kilocode/kilocode.py index c5a5205..416b0bb 100644 --- a/scripts/coding_discovery_tools/windows/kilocode/kilocode.py +++ b/scripts/coding_discovery_tools/windows/kilocode/kilocode.py @@ -67,12 +67,18 @@ def detect(self) -> Optional[Dict]: Returns: Dict containing tool info (name, version, install_path) or None if not found """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._check_user_for_kilocode(Path(scoped_home)) + # When running as administrator, scan all user directories first if self._is_running_as_admin(): user_kilocode_info = self._scan_user_directories() if user_kilocode_info: return user_kilocode_info - + # Check current user (works for both admin and regular users) return self._check_user_for_kilocode(Path.home()) diff --git a/scripts/coding_discovery_tools/windows/roo_code/roo_code.py b/scripts/coding_discovery_tools/windows/roo_code/roo_code.py index f0cb83a..097ab96 100644 --- a/scripts/coding_discovery_tools/windows/roo_code/roo_code.py +++ b/scripts/coding_discovery_tools/windows/roo_code/roo_code.py @@ -66,6 +66,12 @@ def detect(self) -> Optional[List[Dict]]: List of dicts containing tool info for each IDE with Roo Code installed, or None if not found in any IDE """ + # Per-user scan (user_home set by detect_tool_for_user): scope to THIS user only, else an + # elevated scan enumerates every home and attributes other users' extensions to the caller. + scoped_home = getattr(self, 'user_home', None) + if scoped_home is not None: + return self._detect_roo_for_user(Path(scoped_home)) or None + all_results = [] if is_running_as_admin(): diff --git a/tests/test_scan_completed_manifest.py b/tests/test_scan_completed_manifest.py index 3f5f2c6..be5b467 100644 --- a/tests/test_scan_completed_manifest.py +++ b/tests/test_scan_completed_manifest.py @@ -380,13 +380,11 @@ def test_generic_read_error_does_not_fail_close(self): self.assertEqual(captured.get("covered_home_users"), ["alice"]) def test_detector_error_sends_no_manifest(self): - # A detector error means presence is unknown this run, so NO manifest is sent — atomically - # on the completed event — and the backend (seeing no manifest) skips pruning entirely. + # A detector error means presence is unknown this run, so NO manifest AND no covered scope + # are sent — the backend then has no partial inventory/scope to prune from. captured = self._run_main_capture_manifest(detector_failure="ToolGhost") - self.assertIsNone( - captured["manifest"], - "a detector error must send no manifest so the backend can't prune from a partial scan", - ) + self.assertIsNone(captured["manifest"], "detector error must send no manifest") + self.assertIsNone(captured.get("covered_home_users"), "no covered scope without an inventory") def test_per_user_detection_no_phantom_ownership(self): # Phantom-ownership regression: all_tools is deduped globally, so a user-scoped tool one @@ -454,6 +452,21 @@ def _send_scan_event(domain, api_key, device_id, run_id, scan_event, app_name=No self.assertNotIn(("alice", "ToolB"), pairs) +class TestExtensionDetectorPerUserScoping(unittest.TestCase): + """Extension detectors must scope to the per-user home set by detect_tool_for_user, so an + elevated multi-user scan can't attribute one user's extension to another (phantom ownership).""" + + def test_roo_detect_scopes_to_set_user_home(self): + from scripts.coding_discovery_tools.macos.roo_code.roo_code import MacOSRooDetector + det = MacOSRooDetector() + det.user_home = Path("/Users/alice") + with patch.object(det, "_detect_roo_for_user", return_value=[{"name": "Roo Code (Cursor)"}]) as m: + result = det.detect() + # Called exactly once with the SET home (not Path.home(), not per-/Users enumeration). + m.assert_called_once_with(Path("/Users/alice")) + self.assertEqual(result, [{"name": "Roo Code (Cursor)"}]) + + class TestJetBrainsNamingDeterminism(unittest.TestCase): """The JetBrains tool name is the backend prune key (matched exactly vs the manifest), so it must exclude version and license/plan — otherwise a version bump or Free<->Licensed change