From de510b739252710510bff1897a82bb807838ae81 Mon Sep 17 00:00:00 2001 From: Somya-jain0811 Date: Thu, 29 Oct 2026 12:39:07 +0530 Subject: [PATCH] fix: replace broad substring matching in stop/status with strict process detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old is_concore check ('concore' in cmdline) was dangerously broad — it matched editors, git, IDEs, and anything else that happened to touch a file with 'concore' in its path. Changes: - Extract shared detection logic into process_utils.py - Use regex patterns that only match actual concore executions (python concore_*.py, python -m concore, concore CLI commands) - Trust the PID registry (concorekill_pids.txt) as primary source - Detect concorekill.bat as a concore-owned process - Update stop.py and status.py to use the new strict detector - Add comprehensive tests covering true positives, false positives, PID registry, and edge cases --- concore_cli/commands/process_utils.py | 119 ++++++++++++++++++++++++++ concore_cli/commands/status.py | 11 +-- concore_cli/commands/stop.py | 6 +- tests/test_process_detection.py | 112 ++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 7 deletions(-) create mode 100644 concore_cli/commands/process_utils.py create mode 100644 tests/test_process_detection.py diff --git a/concore_cli/commands/process_utils.py b/concore_cli/commands/process_utils.py new file mode 100644 index 0000000..0f9ea14 --- /dev/null +++ b/concore_cli/commands/process_utils.py @@ -0,0 +1,119 @@ +"""Shared concore process detection logic. + +The old substring check ("concore" in cmdline) was dangerously broad — it +matched editors, git, IDEs, and anything else that happened to touch a file +with "concore" in its path. This module provides a strict detector that +won't nuke your VS Code session when you run ``concore stop``. + +See: https://github.com/ControlCore-Project/concore/issues/XXX +""" + +import ntpath +import os +import re +import glob + +# Matches command lines that are actually *running* concore study code: +# python concore_xxx.py … | python -m concore … | python3 ./concore.py … +# Does NOT match: code concore/README.md | git -C concore pull +_CONCORE_EXEC_RE = re.compile( + r""" + (?:python[23w]?[\d.]*) # python interpreter (python, python3, python3.11, pythonw …) + (?:\.exe)? # optional .exe on Windows + \s+.*? # optional flags like -u, -m, etc. + (?: # the actual concore entry point — one of: + concore(?:_\w+)?\.py # concore.py, concore_plant.py, concore_controller.py … + | -m\s+concore # python -m concore … + ) + """, + re.IGNORECASE | re.VERBOSE, +) + +# Known concore CLI entry points — matched as standalone tokens so +# "concore stop" or "concore build" are caught but "vim concore/foo" is not. +_CLI_TOKEN_RE = re.compile( + r"(?:^|\s|[/\\])concore(?:\.exe)?\s+(?:stop|status|build|watch|init|validate|doctor|setup|inspect|editor)", + re.IGNORECASE, +) + +# PID registry written by concore.py at study startup +_PID_REGISTRY_FILENAME = "concorekill_pids.txt" + + +def _load_registered_pids(): + """Collect PIDs from all concorekill_pids.txt files found under cwd.""" + pids = set() + # search current dir and one level deep (covers typical study layouts) + for pattern in [_PID_REGISTRY_FILENAME, os.path.join("*", _PID_REGISTRY_FILENAME)]: + for path in glob.glob(pattern): + try: + with open(path, "r") as f: + for line in f: + stripped = line.strip() + if stripped.isdigit(): + pids.add(int(stripped)) + except OSError: + continue + return pids + + +def _has_concore_markers(cwd): + """True if cwd contains concore runtime files (concore.iport + concore.py). + + mkconcore.py copies these into every generated study node directory, + so their presence is a reliable signal that the process is a real + concore node — not an editor or git operation. + """ + if not cwd: + return False + try: + if not os.path.isfile(os.path.join(cwd, "concore.iport")): + return False + return os.path.isfile(os.path.join(cwd, "concore.py")) or os.path.isfile( + os.path.join(cwd, "concoredocker.py") + ) + except OSError: + return False + + +def is_concore_study_process(proc_info, registered_pids=None, cwd=None): + """Return True only if *proc_info* is genuinely a concore study process. + + Detection priority: + 1. PID found in the concorekill_pids.txt registry (most reliable) + 2. Process is running concorekill.bat (Windows kill script) + 3. Command line matches a concore execution pattern (regex, not substring) + 4. CLI invocation (concore stop, concore build, …) + 5. Working directory contains concore runtime markers (concore.iport) + + Args: + proc_info: dict with keys "pid", "name", "cmdline" (from psutil). + registered_pids: optional pre-loaded set of ints from the PID registry. + cwd: optional working directory of the process (for marker-file check). + """ + pid = proc_info.get("pid") + cmdline = proc_info.get("cmdline") or [] + cmdline_str = " ".join(cmdline) if cmdline else "" + + # 1. Trust the PID registry — concore.py registers every study process + if registered_pids and pid in registered_pids: + return True + + # 2. concorekill.bat — ntpath.basename handles both / and \ separators + if any(ntpath.basename(str(arg)).lower() == "concorekill.bat" for arg in cmdline): + return True + + # 3. Strict regex: must look like an actual concore execution + if _CONCORE_EXEC_RE.search(cmdline_str): + return True + + # 4. CLI invocation (concore stop, concore build, etc.) — but NOT the + # current process (already filtered by callers via current_pid check) + if _CLI_TOKEN_RE.search(cmdline_str): + return True + + # 5. Working dir has concore runtime files (concore.iport + concore.py) + if _has_concore_markers(cwd): + return True + + return False diff --git a/concore_cli/commands/status.py b/concore_cli/commands/status.py index 94c8bc0..616798b 100644 --- a/concore_cli/commands/status.py +++ b/concore_cli/commands/status.py @@ -4,7 +4,7 @@ from rich.panel import Panel from datetime import datetime -from concore_cli.commands._process_match import is_concore_process +from .process_utils import is_concore_study_process, _load_registered_pids def show_status(console): @@ -14,6 +14,7 @@ def show_status(console): try: current_pid = os.getpid() + registered_pids = _load_registered_pids() for proc in psutil.process_iter( ["pid", "name", "cmdline", "create_time", "memory_info", "cpu_percent"] @@ -24,14 +25,13 @@ def show_status(console): if proc.info["pid"] == current_pid: continue - cmdline_str = " ".join(cmdline) if cmdline else "" - try: cwd = proc.cwd() except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): cwd = None - if is_concore_process(cmdline, cwd): + # strict check — won't match editors or git touching concore files + if is_concore_study_process(proc.info, registered_pids, cwd): try: create_time = datetime.fromtimestamp(proc.info["create_time"]) uptime = datetime.now() - create_time @@ -49,8 +49,9 @@ def show_status(console): # Failed to get memory info mem_str = "unknown" + cmdline_joined = " ".join(cmdline) if cmdline else "" command = ( - " ".join(cmdline[:3]) if len(cmdline) >= 3 else cmdline_str[:50] + " ".join(cmdline[:3]) if len(cmdline) >= 3 else cmdline_joined[:50] ) concore_processes.append( diff --git a/concore_cli/commands/stop.py b/concore_cli/commands/stop.py index 168eb87..1852240 100644 --- a/concore_cli/commands/stop.py +++ b/concore_cli/commands/stop.py @@ -4,7 +4,7 @@ import sys from rich.panel import Panel -from concore_cli.commands._process_match import is_concore_process +from .process_utils import is_concore_study_process, _load_registered_pids def stop_all(console): @@ -12,6 +12,7 @@ def stop_all(console): processes_to_kill = [] current_pid = os.getpid() + registered_pids = _load_registered_pids() try: for proc in psutil.process_iter(["pid", "name", "cmdline"]): @@ -26,7 +27,8 @@ def stop_all(console): except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): cwd = None - if is_concore_process(cmdline, cwd): + # strict check — won't match editors or git touching concore files + if is_concore_study_process(proc.info, registered_pids, cwd): processes_to_kill.append(proc) except (psutil.NoSuchProcess, psutil.AccessDenied): # Process already exited or access denied; continue diff --git a/tests/test_process_detection.py b/tests/test_process_detection.py new file mode 100644 index 0000000..db5d233 --- /dev/null +++ b/tests/test_process_detection.py @@ -0,0 +1,112 @@ +"""Tests for the stricter concore process detection logic. + +Verifies that is_concore_study_process() correctly identifies real concore +study processes while rejecting false positives like editors, git, and IDEs +that happen to have "concore" in their command line. + +Import bypasses concore_cli.__init__ (which pulls in click) so the test +runs even when click isn't installed — keeps the CI matrix lean. +""" + +import sys +import os +import importlib +import pytest + +# direct import avoids the concore_cli.__init__ → click dependency chain +_mod_path = os.path.join( + os.path.dirname(__file__), os.pardir, + "concore_cli", "commands", "process_utils.py", +) +spec = importlib.util.spec_from_file_location("process_utils", _mod_path) +process_utils = importlib.util.module_from_spec(spec) +spec.loader.exec_module(process_utils) + +is_concore_study_process = process_utils.is_concore_study_process + + +# -- Processes that SHOULD be detected (true positives) -- + +@pytest.mark.parametrize("cmdline,desc", [ + (["python", "concore_plant.py"], "typical study node"), + (["python3", "concore_controller.py", "--port", "5555"], "study node with args"), + (["python", "-u", "concore.py"], "unbuffered concore.py"), + (["python.exe", "concore.py"], "Windows python.exe"), + (["python3.11", "concore_sensor.py"], "versioned python"), + (["python", "-m", "concore", "run"], "module invocation"), + (["C:\\Python311\\python.exe", "concore_observer.py"], "full path python"), + (["concorekill.bat"], "Windows kill script"), + (["cmd.exe", "/c", "concorekill.bat"], "kill script via cmd"), + (["concore", "build", "workflow.graphml"], "CLI build command"), + (["concore", "stop"], "CLI stop command"), + (["concore", "watch", "./study"], "CLI watch command"), +]) +def test_true_positives(cmdline, desc): + """Real concore processes must always be detected.""" + proc = {"pid": 1234, "name": "python", "cmdline": cmdline} + assert is_concore_study_process(proc) is True, f"missed: {desc}" + + +# -- Processes that MUST NOT be detected (false positives the old code hit) -- + +@pytest.mark.parametrize("cmdline,desc", [ + (["code", "concore/README.md"], "VS Code editing concore file"), + (["vim", "/home/user/concore/concore_base.py"], "vim editing concore source"), + (["git", "-C", "concore", "pull"], "git pull inside concore repo"), + (["git", "log", "--oneline", "concore/stop.py"], "git log on concore file"), + (["grep", "-r", "concore", "./src"], "grep searching for concore"), + (["cat", "/tmp/concore_debug.log"], "reading a concore log file"), + (["less", "concore/README.md"], "less viewing concore docs"), + (["find", ".", "-name", "concore*"], "find searching for concore files"), + (["notepad.exe", "C:\\repos\\concore\\concore.py"], "notepad editing concore"), + (["explorer.exe", "C:\\repos\\concore"], "Windows explorer in concore dir"), +]) +def test_false_positives_rejected(cmdline, desc): + """Editors, git, and tools touching concore files must NOT be killed.""" + proc = {"pid": 5678, "name": cmdline[0], "cmdline": cmdline} + assert is_concore_study_process(proc) is False, f"false positive: {desc}" + + +# -- PID registry takes priority -- + +def test_pid_registry_match(): + """A PID in the registry should be detected even with an empty cmdline.""" + proc = {"pid": 9999, "name": "python", "cmdline": []} + assert is_concore_study_process(proc, registered_pids={9999}) is True + + +def test_pid_registry_miss(): + """A PID NOT in the registry with innocent cmdline should be rejected.""" + proc = {"pid": 1111, "name": "code", "cmdline": ["code", "concore/foo.py"]} + assert is_concore_study_process(proc, registered_pids={9999}) is False + + +# -- Edge cases -- + +def test_empty_cmdline(): + """Process with no cmdline (zombie, kernel thread) should not match.""" + proc = {"pid": 42, "name": "python", "cmdline": []} + assert is_concore_study_process(proc) is False + + +def test_none_cmdline(): + """Process where psutil returns None for cmdline.""" + proc = {"pid": 42, "name": "python", "cmdline": None} + assert is_concore_study_process(proc) is False + + +# -- cwd marker-file detection -- + +def test_cwd_markers_match(tmp_path): + """Process in a dir with concore runtime files should be detected.""" + # create the marker files mkconcore.py generates for study nodes + (tmp_path / "concore.iport").touch() + (tmp_path / "concore.py").touch() + proc = {"pid": 7777, "name": "python", "cmdline": ["python", "some_script.py"]} + assert is_concore_study_process(proc, cwd=str(tmp_path)) is True + + +def test_cwd_markers_missing(): + """Process with no cwd (or non-study dir) should not match on markers.""" + proc = {"pid": 7777, "name": "python", "cmdline": ["python", "some_script.py"]} + assert is_concore_study_process(proc, cwd=None) is False