Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
115 changes: 45 additions & 70 deletions src/normal_day.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,6 @@ def _handle_ticket_progress(
tags=["jira", "blocker"],
)
)

# Update ticket comment and status
ticket.setdefault("comments", []).append(
{
"author": assignee,
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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,
)
Expand Down
172 changes: 165 additions & 7 deletions tests/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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]

Expand Down
Loading
Loading