diff --git a/CHANGELOG.md b/CHANGELOG.md index 99e90df..8de627b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,26 @@ Versioning follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). --- +## [v1.2.7] — 2026-03-25 + +### Added + +- **Stale PR Escape Hatch (`src/normal_day.py`, `tests/test_integration.py`)**: Introduced a time-based override for blocked Pull Requests. PRs that have been stuck with "changes requested" for 7 or more days are now eligible for force-merging, preventing deadlocks when reviewers become unresponsive. +- **Feedback Regression Suite (`tests/test_integration.py`, `tests/test_normal_day.py`)**: Added targeted test cases to ensure PRs with active feedback are not prematurely merged and that addressing feedback correctly resets the review lifecycle. + +### Changed + +- **PR Lifecycle Automation (`src/normal_day.py`)**: Refined the `_complete_eng_ticket` logic to automatically clear the `changes_requested` flag and revert ticket status to "In Review" when an assignee pushes updates, streamlining the re-review process. +- **Memory Operations Efficiency (`src/normal_day.py`)**: Optimized the `_try_force_merge_stale_pr` method by replacing redundant database lookups with a more efficient list comprehension and conditional check for open PRs. + +### Fixed + +- **Duplicate PR Prevention (`src/normal_day.py`)**: Fixed a bug where the system could attempt to spawn a new PR for a ticket that already had an open PR with requested changes. +- **Worker Redundancy (`src/normal_day.py`)**: Standardized artifact embedding by removing conditional checks for `_embed_worker`, consolidating all Jira and comment embedding directly through `_mem.embed_artifact` to reduce logic branching. +- **Logic Guard for Requested Changes (`src/normal_day.py`)**: Corrected a flaw in the force-merge logic that previously ignored the `changes_requested` state, ensuring authors must address feedback before the 5-day automated merge threshold applies. + +--- + ## [v1.2.6] — 2026-03-25 ### Added diff --git a/src/normal_day.py b/src/normal_day.py index bbfc13d..1faadd2 100644 --- a/src/normal_day.py +++ b/src/normal_day.py @@ -350,8 +350,6 @@ def _handle_ticket_progress( tags=["jira", "blocker"], ) ) - - # Update ticket comment and status ticket.setdefault("comments", []).append( { "author": assignee, @@ -427,6 +425,14 @@ def _handle_ticket_progress( self._save_ticket(ticket) + for pr_id in ticket.get("linked_prs", []): + pr = self._mem._prs.find_one({"pr_id": pr_id, "status": "open"}, {"_id": 0}) + if pr and pr.get("changes_requested"): + pr["changes_requested"] = False + ticket["status"] = "In Review" + ticket["in_review_since"] = self._state.day + self._mem.upsert_pr(pr) + ticket_body = "\n".join( filter( None, @@ -438,69 +444,37 @@ def _handle_ticket_progress( ], ) ) - if self._embed_worker: - self._embed_worker.enqueue( - id=ticket_id, - type="jira", - title=ticket.get("title", ticket_id), - content=ticket_body, - day=self._state.day, - date=date_str, - timestamp=current_actor_time_iso, - metadata={ - "assignee": ticket.get("assignee", ""), - "status": ticket["status"], - "dept_type": dept_type, - }, - ) - else: - self._mem.embed_artifact( - id=ticket_id, - type="jira", - title=ticket.get("title", ticket_id), - content=ticket_body, - day=self._state.day, - date=date_str, - timestamp=current_actor_time_iso, - metadata={ - "assignee": ticket.get("assignee", ""), - "status": ticket["status"], - "dept_type": dept_type, - }, - ) - if self._embed_worker: - self._embed_worker.enqueue( - id=comment_id, - type="jira_comment", - title=f"Comment on {ticket_id}", - content=comment_text, - day=self._state.day, - date=date_str, - timestamp=current_actor_time_iso, - metadata={ - "ticket_id": ticket_id, - "author": assignee, - "dept_type": dept_type, - }, - ) - else: - self._mem.embed_artifact( - id=comment_id, - type="jira_comment", - title=f"Comment on {ticket_id}", - content=comment_text, - day=self._state.day, - date=date_str, - timestamp=current_actor_time_iso, - metadata={ - "ticket_id": ticket_id, - "author": assignee, - "dept_type": dept_type, - }, - ) + self._mem.embed_artifact( + id=ticket_id, + type="jira", + title=ticket.get("title", ticket_id), + content=ticket_body, + day=self._state.day, + date=date_str, + timestamp=current_actor_time_iso, + metadata={ + "assignee": ticket.get("assignee", ""), + "status": ticket["status"], + "dept_type": dept_type, + }, + ) + + self._mem.embed_artifact( + id=comment_id, + type="jira_comment", + title=f"Comment on {ticket_id}", + content=comment_text, + day=self._state.day, + date=date_str, + timestamp=current_actor_time_iso, + metadata={ + "ticket_id": ticket_id, + "author": assignee, + "dept_type": dept_type, + }, + ) - # Build artifacts dict and SimEvent facts artifacts = {"jira": ticket_id, "jira_comment": comment_id} if spawned_pr_id: artifacts["pr"] = spawned_pr_id @@ -580,8 +554,8 @@ def _try_force_merge_stale_pr( force_merge = ( review_age >= 5 and bool(linked_prs) - and not open_pr_with_changes and actor_clock_ok + and (not open_pr_with_changes or review_age >= 7) ) if force_merge: @@ -590,13 +564,14 @@ def _try_force_merge_stale_pr( stale_pr = next( ( - self._mem._prs.find_one( - {"pr_id": p, "status": "open"}, {"_id": 0} - ) + doc for p in linked_prs - if self._mem._prs.find_one( - {"pr_id": p, "status": "open"}, {"_id": 0} - ) + for doc in [ + self._mem._prs.find_one( + {"pr_id": p, "status": "open"}, {"_id": 0} + ) + ] + if doc ), None, ) diff --git a/tests/test_integration.py b/tests/test_integration.py index 2c4baf4..aba8456 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -511,12 +511,175 @@ def test_pr_review_fallback_finds_reviewable_pr( assert len(events) == 1 assert events[0]["facts"]["reviewer"] == reviewer + @patch("normal_day.Crew") + @patch("normal_day.Task") + @patch("agent_factory.Agent") + def test_stale_pr_with_changes_requested_is_never_force_merged( + self, mock_agent, mock_task, mock_crew, sim + ): + """ + Bug regression: a PR with changes_requested=True must NOT be force-merged + by _try_force_merge_stale_pr, even when review_age >= 5. + Without the fix this test exposes, the PR stays open indefinitely. + """ + author = ALL_NAMES[0] + _seed_ticket( + sim._mem, "ENG-110", author, status="In Review", linked_prs=["PR-110"] + ) + _seed_pr(sim._mem, "PR-110", "ENG-110", author, [ALL_NAMES[1]]) + + sim._mem._prs.update_one( + {"pr_id": "PR-110"}, {"$set": {"changes_requested": True}} + ) + + sim._mem._jira.update_one( + {"id": "ENG-110"}, {"$set": {"in_review_since": sim.state.day - 6}} + ) + mock_crew.return_value = _make_ticket_progress_crew() + + org_plan = _make_org_plan( + sim, + { + "Engineering_Mobile": [ + EngineerDayPlan( + name=author, + dept="Engineering_Mobile", + agenda=[ + AgendaItem( + activity_type="ticket_progress", + description="Still working on ENG-110", + related_id="ENG-110", + estimated_hrs=2.0, + ) + ], + stress_level=20, + ) + ] + }, + ) + + sim._normal_day.handle(org_plan) + + pr = sim._mem._prs.find_one({"pr_id": "PR-110"}, {"_id": 0}) + assert pr["status"] == "open", ( + "PR with changes_requested must NOT be force-merged — " + "the author must address the feedback first" + ) + ticket = sim._mem.get_ticket("ENG-110") + assert ticket["status"] != "Done", ( + "Ticket must not transition to Done while its PR has changes_requested" + ) + + @patch("normal_day.Crew") + @patch("normal_day.Task") + @patch("agent_factory.Agent") + def test_assignee_progress_clears_changes_requested_flag( + self, mock_agent, mock_task, mock_crew, sim + ): + """ + After the assignee works on a ticket that has a PR with changes_requested, + that flag must be cleared so the PR becomes eligible for force-merge again. + """ + author = ALL_NAMES[0] + _seed_ticket( + sim._mem, "ENG-111", author, status="In Review", linked_prs=["PR-111"] + ) + _seed_pr(sim._mem, "PR-111", "ENG-111", author, [ALL_NAMES[1]]) + sim._mem._prs.update_one( + {"pr_id": "PR-111"}, {"$set": {"changes_requested": True}} + ) + mock_crew.return_value = _make_ticket_progress_crew() + + org_plan = _make_org_plan( + sim, + { + "Engineering_Mobile": [ + EngineerDayPlan( + name=author, + dept="Engineering_Mobile", + agenda=[ + AgendaItem( + activity_type="ticket_progress", + description="Addressing review feedback on ENG-111", + related_id="ENG-111", + estimated_hrs=2.0, + ) + ], + stress_level=20, + ) + ] + }, + ) + + sim._normal_day.handle(org_plan) + + pr = sim._mem._prs.find_one({"pr_id": "PR-111"}, {"_id": 0}) + assert not pr.get("changes_requested"), ( + "Assignee addressing feedback must clear changes_requested " + "so the PR can re-enter the review/force-merge cycle" + ) + + @patch("normal_day.Crew") + @patch("normal_day.Task") + @patch("agent_factory.Agent") + def test_stale_pr_with_changes_requested_force_merges_after_escape_hatch( + self, mock_agent, mock_task, mock_crew, sim + ): + """ + A PR stuck with changes_requested for >= 7 days must eventually be + force-merged via the time-based escape hatch (review_age >= 7 overrides + the changes_requested guard). Without this, a PR whose reviewer goes + silent is blocked forever. + """ + author = ALL_NAMES[0] + _seed_ticket( + sim._mem, "ENG-112", author, status="In Review", linked_prs=["PR-112"] + ) + _seed_pr(sim._mem, "PR-112", "ENG-112", author, [ALL_NAMES[1]]) + sim._mem._prs.update_one( + {"pr_id": "PR-112"}, {"$set": {"changes_requested": True}} + ) + + sim._mem._jira.update_one( + {"id": "ENG-112"}, {"$set": {"in_review_since": sim.state.day - 8}} + ) + mock_crew.return_value = _make_pr_review_crew() + + org_plan = _make_org_plan( + sim, + { + "Engineering_Mobile": [ + EngineerDayPlan( + name=author, + dept="Engineering_Mobile", + agenda=[ + AgendaItem( + activity_type="ticket_progress", + description="Force merge after reviewer silence", + related_id="ENG-112", + estimated_hrs=0.5, + ) + ], + stress_level=20, + ) + ] + }, + ) + + sim._normal_day.handle(org_plan) + + pr = sim._mem._prs.find_one({"pr_id": "PR-112"}, {"_id": 0}) + assert pr["status"] == "merged", ( + "PR stuck with changes_requested for >= 7 days must be force-merged " + "via the time-based escape hatch" + ) + ticket = sim._mem.get_ticket("ENG-112") + assert ticket["status"] == "Done" + class TestPlannerInReviewSection: """ Unit tests for the prompt-building layer — no LLM, no NormalDayHandler. - Catches the data-pipeline bug where in_review_section only emitted ticket - IDs and omitted PR IDs and reviewer names. """ def test_in_review_section_includes_pr_id_and_reviewers(self, sim): @@ -561,11 +724,6 @@ def test_in_review_section_includes_pr_id_and_reviewers(self, sim): assert reviewer in section, "Reviewer name must appear in in_review_section" def test_in_review_section_bare_ticket_id_is_insufficient(self, sim): - """ - Documents the OLD broken behaviour: bare ticket ID gives the LLM - nothing to route on. Asserts the old logic omits what it should include. - This test should FAIL if someone reverts to the old one-liner. - """ author = ALL_NAMES[0] reviewer = ALL_NAMES[1] if len(ALL_NAMES) > 1 else ALL_NAMES[0] diff --git a/tests/test_normal_day.py b/tests/test_normal_day.py index cea3897..9fea35b 100644 --- a/tests/test_normal_day.py +++ b/tests/test_normal_day.py @@ -950,3 +950,111 @@ def test_dept_of_name_returns_first_match_when_name_in_multiple_depts(): result = dept_of_name("Alice", ambiguous_chart) assert result in ("Engineering", "Platform") + + +def test_try_force_merge_skips_pr_with_changes_requested(handler, mock_state): + """ + _try_force_merge_stale_pr must return False (no merge) when the linked PR + has changes_requested=True, even if review_age >= 5. + This is the unit-level regression for the infinite-loop bug. + """ + ticket = _make_ticket( + "ORG-200", "Stuck ticket", status="In Review", assignee="Alice" + ) + ticket["linked_prs"] = ["PR-200"] + ticket["in_review_since"] = mock_state.day - 6 # age = 6, over threshold + + handler._mem.get_ticket = MagicMock(return_value=ticket) + # Simulate PR with changes_requested set + handler._mem._prs.find_one = MagicMock( + return_value={"pr_id": "PR-200", "status": "open", "changes_requested": True} + ) + mock_state.active_incidents = [] + + result = handler._try_force_merge_stale_pr(ticket, "Alice", "2026-01-05") + + assert result is False, ( + "_try_force_merge_stale_pr must not merge a PR that still has " + "changes_requested — the author needs to address the feedback first" + ) + + +def test_try_force_merge_clears_when_changes_addressed(handler, mock_state, clock): + """ + When the PR's changes_requested flag has been cleared (author pushed a fix), + _try_force_merge_stale_pr must proceed to merge after the age threshold. + """ + + mock_state.current_date = datetime(2026, 1, 5) + mock_state.actor_cursors = {} + clock._set_cursor("Alice", datetime(2026, 1, 5, 9, 0)) + + ticket = _make_ticket( + "ORG-201", "Fixed ticket", status="In Review", assignee="Alice" + ) + ticket["linked_prs"] = ["PR-201"] + ticket["in_review_since"] = mock_state.day - 5 + + pr_doc = { + "pr_id": "PR-201", + "title": "Fix something", + "status": "open", + "changes_requested": False, + "author": "Alice", + "linked_ticket": "ORG-201", + } + + def find_one_side_effect(query, *args, **kwargs): + if query.get("changes_requested") is True: + return None + return pr_doc + + handler._mem._prs.find_one = MagicMock(side_effect=find_one_side_effect) + handler._mem.upsert_pr = MagicMock() + handler._git.merge_pr = MagicMock() + mock_state.active_incidents = [] + + with ( + patch.object(handler, "_handle_pr_review_for_incident"), + patch.object(handler, "_emit_bot_message"), + patch.object(handler, "_save_ticket"), + ): + result = handler._try_force_merge_stale_pr(ticket, "Alice", "2026-01-05") + + assert result is True + handler._git.merge_pr.assert_called_once_with("PR-201") + + +def test_complete_eng_ticket_does_not_spawn_pr_when_changes_requested( + handler, mock_state +): + """ + _complete_eng_ticket must not spawn a new PR when the existing linked PR + still has changes_requested=True. The assignee should address the existing + review, not open a duplicate PR. + """ + ticket = _make_ticket( + "ORG-202", "In-flight ticket", status="In Review", assignee="Bob" + ) + ticket["linked_prs"] = ["PR-202"] + ticket["in_progress_since"] = mock_state.day - 4 + + handler._mem._prs.find_one = MagicMock( + return_value={"pr_id": "PR-202", "status": "open", "changes_requested": True} + ) + + with patch.object(handler._git, "create_pr") as mock_create_pr: + handler._complete_eng_ticket( + ticket=ticket, + assignee="Bob", + actor_incident_bound=False, + date_str="2026-01-05", + timestamp_iso="2026-01-05T10:00:00", + chain=MagicMock(), + is_complete=True, + ) + + ( + mock_create_pr.assert_not_called(), + ("Must not open a new PR when an existing one already has changes_requested"), + )