Fix edge cases, expand test coverage to 62

Fix conflict regex to match plurals, sort run IDs
numerically. Add tests for URL variants, word
boundaries, timezone offsets, parser validation.

Author: stephen-tatari

pr-status/scripts/pr_status.py

@@ -133,7 +133,7 @@ def extract_run_ids_from_checks(checks: list[dict]) -> list[str]:
         m = re.search(r"/actions/runs/(\d+)", link)
         if m:
             run_ids.add(m.group(1))
-    return sorted(run_ids)
+    return sorted(run_ids, key=int)
 
 
 def merge_state_message(state: str) -> str:
@@ -154,7 +154,7 @@ def find_error_keywords(body: str) -> list[str]:
         ("error", r"(?i)\berror\b"),
         ("failure", r"(?i)\bfail(?:ed|ure|ing)?\b"),
         ("warning", r"(?i)\bwarn(?:ing)?\b"),
-        ("conflict", r"(?i)\bconflict\b"),
+        ("conflict", r"(?i)\bconflicts?\b"),
         ("deprecated", r"(?i)\bdeprecated?\b"),
         ("vulnerability", r"(?i)\bvulnerabilit(?:y|ies)\b"),
         ("breaking", r"(?i)\bbreaking\b"),

pr-status/scripts/test_pr_status.py

@@ -10,6 +10,7 @@
 import pytest
 
 from pr_status import (
+    build_parser,
     classify_bot,
     extract_run_ids_from_checks,
     find_error_keywords,
@@ -75,6 +76,39 @@ def test_invalid_exits(self):
             parse_pr_identifier("not-a-pr")
         assert exc_info.value.code == 2
 
+    def test_url_with_files_suffix(self):
+        pr, repo = parse_pr_identifier(
+            "https://github.com/o/r/pull/123/files",
+        )
+        assert pr == "123"
+        assert repo == "o/r"
+
+    def test_url_with_query_string(self):
+        pr, repo = parse_pr_identifier(
+            "https://github.com/o/r/pull/123?diff=unified",
+        )
+        assert pr == "123"
+        assert repo == "o/r"
+
+    def test_url_with_trailing_slash(self):
+        pr, repo = parse_pr_identifier(
+            "https://github.com/o/r/pull/123/",
+        )
+        assert pr == "123"
+        assert repo == "o/r"
+
+    def test_issue_url_exits(self):
+        """Issue URLs are not PR URLs — should reject."""
+        with pytest.raises(SystemExit) as exc_info:
+            parse_pr_identifier("https://github.com/o/r/issues/123")
+        assert exc_info.value.code == 2
+
+    def test_whitespace_padded_number_exits(self):
+        """Leading/trailing whitespace on a number is not a valid identifier."""
+        with pytest.raises(SystemExit) as exc_info:
+            parse_pr_identifier(" 42 ")
+        assert exc_info.value.code == 2
+
 
 # ---------------------------------------------------------------------------
 # classify_bot
@@ -135,23 +169,55 @@ def test_non_actions_link(self):
     def test_empty_list(self):
         assert extract_run_ids_from_checks([]) == []
 
+    def test_numeric_sort_not_lexicographic(self):
+        """Run IDs sort numerically: 2 before 10, not "10" before "2"."""
+        checks = [
+            {"bucket": "fail", "link": "https://github.com/o/r/actions/runs/10"},
+            {"bucket": "fail", "link": "https://github.com/o/r/actions/runs/2"},
+        ]
+        assert extract_run_ids_from_checks(checks) == ["2", "10"]
+
+    def test_ignores_pending_bucket_with_actions_link(self):
+        """Only 'fail' bucket checks are extracted, even if link is valid."""
+        checks = [
+            {"bucket": "pending", "link": "https://github.com/o/r/actions/runs/999"},
+        ]
+        assert extract_run_ids_from_checks(checks) == []
+
 
 # ---------------------------------------------------------------------------
 # merge_state_message
 # ---------------------------------------------------------------------------
 
 class TestMergeStateMessage:
-    def test_known_states(self):
-        assert "conflict" in merge_state_message("DIRTY").lower()
-        assert "behind" in merge_state_message("BEHIND").lower()
-        assert "blocked" in merge_state_message("BLOCKED").lower()
-        assert "failing" in merge_state_message("UNSTABLE").lower()
-        assert "computing" in merge_state_message("UNKNOWN").lower()
-
-    def test_unknown_state_passthrough(self):
+    def test_dirty(self):
+        assert merge_state_message("DIRTY") == "Branch has merge conflicts that need resolution"
+
+    def test_behind(self):
+        assert merge_state_message("BEHIND") == "Branch is behind base and needs to be updated"
+
+    def test_blocked(self):
+        assert merge_state_message("BLOCKED") == "Merge is blocked by branch protection rules"
+
+    def test_unstable(self):
+        assert merge_state_message("UNSTABLE") == "Some required checks are failing"
+
+    def test_unknown(self):
+        assert merge_state_message("UNKNOWN") == "GitHub is still computing merge status — try again shortly"
+
+    def test_unmapped_state_includes_state_name(self):
         msg = merge_state_message("SOMETHING_NEW")
         assert "SOMETHING_NEW" in msg
 
+    def test_clean_not_in_map(self):
+        """CLEAN is not an error state, so it falls through to default."""
+        msg = merge_state_message("CLEAN")
+        assert "CLEAN" in msg
+
+    def test_has_hooks_not_in_map(self):
+        msg = merge_state_message("HAS_HOOKS")
+        assert "HAS_HOOKS" in msg
+
 
 # ---------------------------------------------------------------------------
 # find_error_keywords
@@ -170,8 +236,10 @@ def test_finds_vulnerability(self):
         assert "vulnerability" in kw
 
     def test_finds_conflict(self):
-        kw = find_error_keywords("Merge conflict detected")
-        assert any("conflict" in k for k in kw)
+        assert "conflict" in find_error_keywords("Merge conflict detected")
+
+    def test_finds_conflicts_plural(self):
+        assert "conflict" in find_error_keywords("Merge conflicts detected")
 
     def test_no_keywords(self):
         assert find_error_keywords("All good, no problems here") == []
@@ -180,7 +248,29 @@ def test_empty_body(self):
         assert find_error_keywords("") == []
 
     def test_case_insensitive(self):
-        assert len(find_error_keywords("WARNING: something")) > 0
+        assert "warning" in find_error_keywords("WARNING: something")
+
+    def test_multiple_keywords_in_defined_order(self):
+        """When multiple keywords match, they appear in definition order."""
+        kw = find_error_keywords("Error: build failed with conflict and warning")
+        assert kw == ["error", "failure", "warning", "conflict"]
+
+    def test_no_duplicates_on_repeated_matches(self):
+        """Each keyword appears at most once even if the word appears multiple times."""
+        kw = find_error_keywords("error error error")
+        assert kw.count("error") == 1
+
+    def test_word_boundary_no_false_positive_terror(self):
+        """'terror' should not trigger 'error' due to word boundary."""
+        assert find_error_keywords("reign of terror") == []
+
+    def test_word_boundary_no_false_positive_warning_substring(self):
+        """'forewarning' should not trigger 'warning' due to word boundary."""
+        assert find_error_keywords("a forewarning sign") == []
+
+    def test_deprecated_singular_and_trailing_d(self):
+        assert "deprecated" in find_error_keywords("This API is deprecated")
+        assert "deprecated" in find_error_keywords("deprecate this function")
 
 
 # ---------------------------------------------------------------------------
@@ -212,6 +302,68 @@ def test_invalid_dates(self):
 
     def test_none_values(self):
         assert is_stale_approval(None, "2025-01-02T10:00:00Z") is False
+        assert is_stale_approval("2025-01-01T10:00:00Z", None) is False
+        assert is_stale_approval(None, None) is False
+
+    def test_timezone_offsets_equivalent_not_stale(self):
+        """Same instant expressed in different timezones should not be stale."""
+        assert is_stale_approval(
+            "2025-01-02T12:00:00+02:00",
+            "2025-01-02T10:00:00Z",
+        ) is False
+
+    def test_timezone_offsets_stale(self):
+        """Earlier instant in positive offset is still stale."""
+        assert is_stale_approval(
+            "2025-01-01T10:00:00+02:00",
+            "2025-01-02T10:00:00Z",
+        ) is True
+
+    def test_fractional_seconds(self):
+        assert is_stale_approval(
+            "2025-01-01T10:00:00.123Z",
+            "2025-01-02T10:00:00.456Z",
+        ) is True
+
+
+# ---------------------------------------------------------------------------
+# build_parser
+# ---------------------------------------------------------------------------
+
+class TestBuildParser:
+    def test_requires_subcommand(self):
+        parser = build_parser()
+        with pytest.raises(SystemExit) as exc_info:
+            parser.parse_args([])
+        assert exc_info.value.code == 2
+
+    def test_analyze_logs_requires_run_id(self):
+        parser = build_parser()
+        with pytest.raises(SystemExit) as exc_info:
+            parser.parse_args(["analyze-logs"])
+        assert exc_info.value.code == 2
+
+    def test_diagnose_accepts_optional_pr(self):
+        parser = build_parser()
+        args = parser.parse_args(["diagnose"])
+        assert args.command == "diagnose"
+        assert args.pr is None
+
+    def test_diagnose_accepts_pr_number(self):
+        parser = build_parser()
+        args = parser.parse_args(["diagnose", "123"])
+        assert args.pr == "123"
+
+    def test_global_repo_flag(self):
+        parser = build_parser()
+        args = parser.parse_args(["--repo", "org/repo", "status", "42"])
+        assert args.repo == "org/repo"
+        assert args.command == "status"
+
+    def test_global_format_flag(self):
+        parser = build_parser()
+        args = parser.parse_args(["--format", "json", "checks"])
+        assert args.format == "json"
 
 
 if __name__ == "__main__":