From 2261a892a21841cda7b799b8be6a48a8e89ff48d Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Mon, 7 Sep 2026 18:46:27 +0100 Subject: [PATCH] fix: guard slice() against non-datetime timestamps and broken symlinks Two unrelated bugs in the ovos-logs CLI: - OVOSLogParser.parse() fell back to timestamp="" (a string) for a log line that fails to match LOG_PATTERN before any timestamped line has been seen. slice() then compared that string against datetime bounds and raised TypeError. parse() now falls back to None (matching LogLine's declared type) and slice() skips entries with no timestamp. - get_log_path() used os.path.exists() to test for a service's log file, which returns False for a broken symlink even though the entry is listed by get_available_logs()'s os.listdir()-based scan. That mismatch let a service that "exists" get None back for its log directory, which then blew up os.path.join(). Switched to os.path.lexists() so both functions agree on what counts as present. Both were reproduced against dev, regression tests added that fail before each fix and pass after. Full existing suite unaffected (1002 passed, 9 pre-existing unrelated failures, before and after). Closes #261 Closes #227 Co-Authored-By: Claude Sonnet 5 --- ovos_utils/log.py | 2 +- ovos_utils/log_parser.py | 4 ++-- test/unittests/test_log.py | 11 +++++++++ test/unittests/test_log_parser.py | 37 +++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/ovos_utils/log.py b/ovos_utils/log.py index 27af6ef5..c286044e 100644 --- a/ovos_utils/log.py +++ b/ovos_utils/log.py @@ -396,7 +396,7 @@ def get_log_path(service: str, directories: Optional[List[str]] = None) \ if directories: for directory in directories: file = os.path.join(directory, f"{service}.log") - if os.path.exists(file): + if os.path.lexists(file): return directory return None diff --git a/ovos_utils/log_parser.py b/ovos_utils/log_parser.py index 4aa3cb8f..555367d9 100644 --- a/ovos_utils/log_parser.py +++ b/ovos_utils/log_parser.py @@ -150,7 +150,7 @@ def parse(cls, log_line, last_timestamp=None) -> LogLine: data['timestamp'] = datetime.strptime(data['timestamp'], TIME_FORMAT) return LogLine(**data) - data["timestamp"] = last_timestamp or "" + data["timestamp"] = last_timestamp data["message"] = log_line return LogLine(**data) @@ -374,7 +374,7 @@ def slice(start, until, logs, paths, file): continue _templog[service] = [] for log in OVOSLogParser.parse_file(logfile): - if start <= log.timestamp < end: + if log.timestamp is not None and start <= log.timestamp < end: if isinstance(log, Traceback): _templog[service].extend(log.to_loglines()) else: diff --git a/test/unittests/test_log.py b/test/unittests/test_log.py index b187eb57..e92a7f6e 100644 --- a/test/unittests/test_log.py +++ b/test/unittests/test_log.py @@ -394,6 +394,17 @@ def test_get_log_path(self, get_config): self.assertEqual(get_log_path("test"), self.test_dir) get_config.assert_called_once_with(service_name="test") + def test_get_log_path_broken_symlink(self): + from ovos_utils.log import get_log_path + + symlink_path = join(self.test_dir, "broken.log") + os.symlink(join(self.test_dir, "does_not_exist.log"), symlink_path) + try: + self.assertEqual(get_log_path("broken", [self.test_dir]), + self.test_dir) + finally: + os.unlink(symlink_path) + @patch('ovos_config.Configuration') def test_get_log_paths(self, config): from ovos_utils.log import get_log_paths diff --git a/test/unittests/test_log_parser.py b/test/unittests/test_log_parser.py index 46ae51d1..4b3cba2d 100644 --- a/test/unittests/test_log_parser.py +++ b/test/unittests/test_log_parser.py @@ -214,6 +214,13 @@ def test_parse_invalid_line_with_last_timestamp(self) -> None: result = OVOSLogParser.parse("some system message\n", last_timestamp=ts) self.assertEqual(result.timestamp, ts) + def test_parse_invalid_line_without_last_timestamp(self) -> None: + """parse should leave timestamp as None, not a string, when there is + no last_timestamp to fall back to.""" + from ovos_utils.log_parser import OVOSLogParser + result = OVOSLogParser.parse("some system message\n") + self.assertIsNone(result.timestamp) + def test_parse_file_valid(self) -> None: """parse_file should yield LogLine objects from a valid log file.""" from ovos_utils.log_parser import OVOSLogParser, LogLine @@ -283,6 +290,36 @@ def test_parse_file_skips_blank_lines(self) -> None: finally: os.unlink(fname) + def test_parse_file_leading_line_without_timestamp(self) -> None: + """A log line that does not match LOG_PATTERN (e.g. it is missing a + field) before any timestamped line is seen must not leave a string + in LogLine.timestamp, since callers compare it against datetimes.""" + from ovos_utils.log_parser import OVOSLogParser + + content = ( + "2024-07-17 21:59:57.530 - common_query.openvoiceos - INFO - " + "First run of common_query.openvoiceos\n" + "2024-07-17 22:00:01.123 - skills - core.MSM - DEBUG - loaded skill\n" + ) + with tempfile.NamedTemporaryFile(mode="w", suffix=".log", delete=False) as f: + f.write(content) + fname = f.name + + try: + results = list(OVOSLogParser.parse_file(fname)) + first, second = results + self.assertIsNone(first.timestamp) + self.assertIsInstance(second.timestamp, datetime) + start = datetime(2024, 1, 1) + end = datetime.now() + # must not raise: comparing None against datetimes previously + # raised TypeError because the fallback timestamp was "" + filtered = [log for log in results + if log.timestamp is not None and start <= log.timestamp < end] + self.assertEqual(len(filtered), 1) + finally: + os.unlink(fname) + class TestParseTime(unittest.TestCase): """Tests for the parse_time helper."""