diff --git a/src/plane_sync.py b/src/plane_sync.py index 4801db2..9d061c4 100644 --- a/src/plane_sync.py +++ b/src/plane_sync.py @@ -197,6 +197,42 @@ def fetch_issue_description(issue_id: str, project_id: str) -> str: return "" +def fetch_issue_fields(issue_id: str, project_id: str) -> tuple[str, str]: + """BUG B: GET the Plane issue by UUID ONCE and return (name, description). + + Plane's ``issue.updated`` webhook (e.g. a status change) only carries the + CHANGED fields, so BOTH ``name`` and ``description`` are usually absent in + the payload. start_pipeline needs the real title (for the branch slug) and + the real description (for the analyst .task.md). To avoid issuing two + separate issue-detail GETs (one for name, one for description), this single + request returns both. + + Reuses the exact GET issue detail endpoint / shared token already used by + ``fetch_issue_sequence_id`` / ``fetch_issue_description``. For the + description it applies the same logic as ``fetch_issue_description`` + (prefer ``description_stripped``, fall back to stripping + ``description_html``). + + Returns ("", "") on network error, non-2xx, or missing body - never raises, + so a Plane outage degrades gracefully (caller keeps its payload fallbacks). + """ + url = f"{PLANE_BASE}/workspaces/{WORKSPACE}/projects/{project_id}/issues/{issue_id}/" + try: + resp = httpx.get(url, headers=PLANE_HEADERS, timeout=10) + resp.raise_for_status() + body = resp.json() + name = (body.get("name") or "").strip() + desc = body.get("description_stripped") + if desc and desc.strip(): + description = desc + else: + description = _strip_html(body.get("description_html") or "") + return name, description + except Exception as e: + logger.warning(f"fetch_issue_fields failed for {issue_id}: {e}") + return "", "" + + def find_issue_id(work_item_id: str, project_id: str = None) -> str | None: """Find Plane issue UUID by work_item_id (e.g. 'ET-002').""" project_id = _resolve_project_id(work_item_id, project_id) diff --git a/src/stage_engine.py b/src/stage_engine.py index bef17a7..3068039 100644 --- a/src/stage_engine.py +++ b/src/stage_engine.py @@ -257,6 +257,58 @@ def advance_stage( return result +def _build_analyst_ready_comment(repo: str, work_item_id: str, branch: str) -> str: + """BUG C: HTML comment posted when analyst artifacts are ready. + + Status-only model (PR #12): approval is the **Approved** status, NOT a + ``:approved:`` comment and NOT moving back to In Progress. The comment asks + the stakeholder to flip the status and links the documents the analyst + actually produced. + + Links point at the Gitea web view: + {gitea_url}/{owner}/{repo}/src/branch/{branch}/docs/work-items/{wid}/ + Only files that REALLY exist in the worktree are listed (no invented docs). + """ + text = ( + "\u2705 BRD/\u0422\u0417/AC \u0433\u043e\u0442\u043e\u0432\u044b. " + "\u0414\u043b\u044f \u043f\u0440\u043e\u0434\u0432\u0438\u0436\u0435\u043d\u0438\u044f " + "\u043f\u0435\u0440\u0435\u0432\u0435\u0434\u0438\u0442\u0435 \u0437\u0430\u0434\u0430\u0447\u0443 " + "\u0432 \u0441\u0442\u0430\u0442\u0443\u0441 Approved. " + "\u0414\u043b\u044f \u043e\u0442\u043a\u043b\u043e\u043d\u0435\u043d\u0438\u044f \u2014 " + "\u043d\u0430\u043f\u0438\u0448\u0438\u0442\u0435 \u043f\u0440\u0438\u0447\u0438\u043d\u0443 " + "\u043a\u043e\u043c\u043c\u0435\u043d\u0442\u043e\u043c \u0438 \u043f\u0435\u0440\u0435\u0432\u0435\u0434\u0438\u0442\u0435 " + "\u0432 Rejected." + ) + + # Candidate analyst artifacts (label -> filename). Only existing ones linked. + candidates = [ + ("Business request", "00-business-request.md"), + ("BRD", "01-brd.md"), + ("\u0422\u0417 (TRZ)", "02-trz.md"), + ("Acceptance Criteria", "03-acceptance-criteria.md"), + ("Test Plan", "04-test-plan.yaml"), + ("UI Test Cases", "04b-ui-test-cases.md"), + ] + rel_dir = f"docs/work-items/{work_item_id}" + try: + wt_dir = os.path.join(get_worktree_path(repo, branch), rel_dir) + except Exception: + wt_dir = None + + owner = getattr(settings, "gitea_owner", "admin") + base = settings.gitea_url.rstrip("/") + links = [] + for label, fname in candidates: + if wt_dir and not os.path.isfile(os.path.join(wt_dir, fname)): + continue + href = f"{base}/{owner}/{repo}/src/branch/{branch}/{rel_dir}/{fname}" + links.append(f'
  • {label}
    • ') + + if links: + text += "
    \u0414\u043e\u043a\u0443\u043c\u0435\u043d\u0442\u044b:" + return text + + def _handle_analysis_approved_flow( task_id, current_stage, repo, work_item_id, branch, agent, result: AdvanceResult ): @@ -279,19 +331,17 @@ def _handle_analysis_approved_flow( files_ok, _ = files_check(repo, work_item_id, branch) if files_ok: - # Full artifacts ready -> In Review, ask for :approved:. + # Full artifacts ready -> In Review, ask for the Approved STATUS (BUG C). set_issue_in_review(work_item_id) plane_add_comment( work_item_id, - "\U0001f4cb BRD/\u0422\u0417/AC/TestPlan \u0433\u043e\u0442\u043e\u0432\u044b. " - "\u041f\u0440\u043e\u0448\u0443 review \u0438 \u0440\u0435\u0430\u043a\u0446\u0438\u044e :approved: " - "\u0434\u043b\u044f \u043f\u0440\u043e\u0434\u0432\u0438\u0436\u0435\u043d\u0438\u044f \u0432 Architecture.", + _build_analyst_ready_comment(repo, work_item_id, branch), author="analyst", ) notify_approve_requested(task_id) result.note = "analysis-in-review" logger.info( - f"Task {task_id}: analyst finished, requested :approved: in Plane" + f"Task {task_id}: analyst finished, requested Approved status in Plane" ) return diff --git a/src/webhooks/plane.py b/src/webhooks/plane.py index 455525b..1eb0f6b 100644 --- a/src/webhooks/plane.py +++ b/src/webhooks/plane.py @@ -387,22 +387,35 @@ async def start_pipeline(data: dict, project_id: str = ""): repo = proj.repo plane_project_id = proj.plane_project_id - # BUG 1: Plane's issue.updated webhook (status change -> In Progress) sends - # only the CHANGED fields, so description / description_stripped are usually - # empty here even though the issue HAS a description. If the payload's - # description is missing/too short, pull the full one from the Plane issue - # detail API (same GET endpoint + shared token already used by - # fetch_issue_sequence_id) before QG-0 runs. If the API is also empty, QG-0 - # legitimately fails (truly empty ticket). - if not description or len(description.strip()) < 20: - from ..plane_sync import fetch_issue_description - fetched = fetch_issue_description(plane_id, plane_project_id) - if fetched and len(fetched.strip()) >= len(description.strip()): - description = fetched + # BUG 1 + BUG B: Plane's issue.updated webhook (status change -> In Progress) + # sends only the CHANGED fields, so BOTH description / description_stripped + # AND name are usually empty here even though the issue HAS them. Pull the + # full title + description from the Plane issue detail API in a SINGLE GET + # (fetch_issue_fields: same endpoint + shared token already used by + # fetch_issue_sequence_id) before QG-0 and before the branch slug is built. + # If the API is also empty, QG-0 legitimately fails (truly empty ticket) and + # name falls back to "untitled". + name_missing = (not name) or name.strip().lower() == "untitled" or len(name.strip()) < 3 + desc_missing = (not description) or len(description.strip()) < 20 + if name_missing or desc_missing: + from ..plane_sync import fetch_issue_fields + fetched_name, fetched_desc = fetch_issue_fields(plane_id, plane_project_id) + if desc_missing and fetched_desc and len(fetched_desc.strip()) >= len(description.strip()): + description = fetched_desc logger.info( f"start_pipeline: pulled description from Plane API for {plane_id} " f"({len(description.strip())} chars)" ) + if name_missing and fetched_name and len(fetched_name.strip()) >= 3: + name = fetched_name + logger.info( + f"start_pipeline: pulled name from Plane API for {plane_id} " + f"('{name}')" + ) + # BUG B fallback: if name is still empty/blank after the API pull, keep the + # legacy "untitled" so the slug/branch build never crashes on an empty name. + if not name or not name.strip(): + name = "untitled" # QG-0 validation (hard gate on pipeline start) errors = _qg0_errors(name, description) @@ -509,7 +522,10 @@ async def start_pipeline(data: dict, project_id: str = ""): task_row = get_db().execute("SELECT id FROM tasks WHERE work_item_id=?", (work_item_id,)).fetchone() if task_row: task_id = task_row[0] - task_desc = f"Work item: {work_item_id}\nRepo: {repo}\nBranch: {branch}\nStage: analysis\nTitle: {name}" + task_desc = ( + f"Work item: {work_item_id}\nRepo: {repo}\nBranch: {branch}\n" + f"Stage: analysis\nTitle: {name}\n\nDescription:\n{description}" + ) job_id = enqueue_job("analyst", repo, task_desc, task_id=task_id) logger.info(f"Task {task_id}: enqueued analyst (job_id={job_id})") # Post start comment to Plane diff --git a/tests/test_analyst_comment.py b/tests/test_analyst_comment.py new file mode 100644 index 0000000..a4e5414 --- /dev/null +++ b/tests/test_analyst_comment.py @@ -0,0 +1,47 @@ +"""BUG C: analyst "artifacts ready" comment under the status-only model. + +The comment must ask for the **Approved** status (not the obsolete +":approved:" reaction, not moving back to "In Progress") and link only the +docs that actually exist in the worktree. +""" + +import os +import tempfile + +os.environ.setdefault("ORCH_GITEA_TOKEN", "test-token") +os.environ.setdefault("ORCH_PLANE_API_TOKEN", "test-token") + + +def test_analyst_comment_asks_approved_with_links(monkeypatch, tmp_path): + from src import stage_engine as SE + + # Worktree with only SOME of the candidate docs present. + wt = tmp_path / "wt" + docs = wt / "docs" / "work-items" / "ET-011" + docs.mkdir(parents=True) + for fname in ("00-business-request.md", "01-brd.md", "02-trz.md", + "03-acceptance-criteria.md", "04-test-plan.yaml"): + (docs / fname).write_text("x") + # 04b-ui-test-cases.md intentionally absent -> must NOT be linked + + monkeypatch.setattr(SE, "get_worktree_path", lambda repo, branch: str(wt)) + monkeypatch.setattr(SE.settings, "gitea_url", "https://git.mva154.duckdns.org") + monkeypatch.setattr(SE.settings, "gitea_owner", "admin") + + html = SE._build_analyst_ready_comment( + "enduro-trails", "ET-011", "feature/ET-011-gpx-upload-feature" + ) + + # text asks for the Approved STATUS, not the obsolete mechanisms + assert "Approved" in html + assert ":approved:" not in html + assert "In Progress" not in html + assert "Rejected" in html + # clickable links to docs that ACTUALLY exist + assert " start_pipeline pulls it from the - Plane API -> QG-0 passes -> task created + analyst enqueued (NOT blocked).""" + Plane API (single fetch_issue_fields GET) -> QG-0 passes -> task created + + analyst enqueued (NOT blocked).""" resp = _to_in_progress_no_desc("bug1") assert resp.status_code == 200 - # description was pulled from the API - mock_desc.assert_called_once() + # name + description were pulled from the API in one call + mock_fields.assert_called_once() # QG-0 passed -> task created and analyst launched (NOT set_issue_blocked) assert _count("bug1") == 1 assert _task("bug1")["stage"] == "analysis" @@ -131,15 +133,15 @@ def test_status_start_fetches_description( @patch("src.webhooks.plane._create_initial_docs", new_callable=AsyncMock) @patch("src.webhooks.plane._create_gitea_branch", new_callable=AsyncMock) @patch("src.plane_sync.fetch_issue_sequence_id", return_value=42) -@patch("src.plane_sync.fetch_issue_description", return_value="") +@patch("src.plane_sync.fetch_issue_fields", return_value=("", "")) def test_status_start_empty_api_still_blocks( - mock_desc, mock_seq, mock_branch, mock_docs, mock_enqueue + mock_fields, mock_seq, mock_branch, mock_docs, mock_enqueue ): """BUG 1 negative path: if the API also returns empty, QG-0 legitimately fails -> NO task is created (truly empty ticket).""" resp = _to_in_progress_no_desc("bug1-empty") assert resp.status_code == 200 - mock_desc.assert_called_once() + mock_fields.assert_called_once() assert _count("bug1-empty") == 0 mock_enqueue.assert_not_called() @@ -168,10 +170,11 @@ def test_work_item_id_uniqueness(): @patch("src.webhooks.plane._create_initial_docs", new_callable=AsyncMock) @patch("src.webhooks.plane._create_gitea_branch", new_callable=AsyncMock) @patch("src.plane_sync.fetch_issue_sequence_id", return_value=6) -@patch("src.plane_sync.fetch_issue_description", - return_value="A sufficiently long description for QG-0 to pass cleanly.") +@patch("src.plane_sync.fetch_issue_fields", + return_value=("Popup enduro trails feature", + "A sufficiently long description for QG-0 to pass cleanly.")) def test_collision_reassigns_in_start_pipeline( - mock_desc, mock_seq, mock_branch, mock_docs, mock_enqueue + mock_fields, mock_seq, mock_branch, mock_docs, mock_enqueue ): """BUG 2a end-to-end: ET-006 already exists -> a new In Progress issue whose Plane sequence_id is also 6 must NOT reuse ET-006.""" diff --git a/tests/test_taskmd_description.py b/tests/test_taskmd_description.py new file mode 100644 index 0000000..d96734e --- /dev/null +++ b/tests/test_taskmd_description.py @@ -0,0 +1,138 @@ +"""Tests for fix/taskmd-description (3 bugs at the analyst pipeline entry/exit): + +BUG A: start_pipeline built the analyst .task.md WITHOUT the description body + (only Title), so analyst received a ~101-byte file and reported the + "business request is empty". task_desc must now carry the description. + +BUG B: issue.updated ships only changed fields, so `name` is usually absent -> + slug/branch became "untitled". start_pipeline must pull the real name + from the Plane API (single fetch_issue_fields GET, above the slug build) + so the branch slug is NOT "untitled". + +BUG C: the analyst "artifacts ready" comment used the obsolete ":approved:" + wording. Under the status-only model it must ask for the **Approved** + status (not ":approved:", not "In Progress") and link the docs that + actually exist. +""" + +import os +import tempfile + +_test_db = os.path.join(tempfile.gettempdir(), "test_orchestrator_taskmd_desc.db") +os.environ["ORCH_DB_PATH"] = _test_db +os.environ.setdefault("ORCH_PLANE_WEBHOOK_SECRET", "") +os.environ.setdefault("ORCH_GITEA_TOKEN", "test-token") +os.environ.setdefault("ORCH_PLANE_API_TOKEN", "test-token") + +import pytest # noqa: E402 +from unittest.mock import patch, AsyncMock # noqa: E402 +from fastapi.testclient import TestClient # noqa: E402 + +from src.main import app # noqa: E402 +from src.db import init_db, get_db # noqa: E402 +from src import projects as P # noqa: E402 +from src.projects import reload_projects # noqa: E402 + +ENDURO_PLANE_ID = "7a79f0a9-5278-49cd-9007-9a338f238f9c" +IN_PROGRESS = "b873d9eb-993c-48cd-97ac-99a9b1623967" +BACKLOG = "113b24f6-cce8-4be9-9a22-a359b9cf0122" + +client = TestClient(app) + + +@pytest.fixture(autouse=True) +def setup(monkeypatch): + monkeypatch.setattr(P.settings, "db_path", _test_db) + import src.db as _db + monkeypatch.setattr(_db.settings, "db_path", _test_db) + if os.path.exists(_test_db): + os.unlink(_test_db) + init_db() + monkeypatch.setattr("src.webhooks.plane.verify_plane_signature", lambda body, sig: True) + registry_json = ( + f'[{{"plane_project_id": "{ENDURO_PLANE_ID}", "repo": "enduro-trails",' + f' "work_item_prefix": "ET", "name": "enduro-trails"}}]' + ) + monkeypatch.setattr(P.settings, "projects_json", registry_json) + reload_projects() + yield + reload_projects() + if os.path.exists(_test_db): + os.unlink(_test_db) + + +def _task(plane_id): + conn = get_db() + row = conn.execute("SELECT * FROM tasks WHERE plane_id=?", (plane_id,)).fetchone() + conn.close() + return row + + +# --------------------------------------------------------------------------- # +# BUG A: description reaches the analyst .task.md +# --------------------------------------------------------------------------- # +@patch("src.webhooks.plane.enqueue_job", return_value=1) +@patch("src.webhooks.plane._create_initial_docs", new_callable=AsyncMock) +@patch("src.webhooks.plane._create_gitea_branch", new_callable=AsyncMock) +@patch("src.plane_sync.fetch_issue_sequence_id", return_value=11) +@patch("src.plane_sync.fetch_issue_fields", + return_value=("ET-011 real title", + "REAL BUSINESS REQUEST BODY: user wants GPX upload with " + "validation and a results map.")) +def test_taskdesc_includes_description( + mock_fields, mock_seq, mock_branch, mock_docs, mock_enqueue +): + resp = client.post("/webhook/plane", json={ + "event": "issue", "action": "updated", + "data": { + "id": "taskA", + # status change payload: NO name, NO description (only changed field) + "project": ENDURO_PLANE_ID, + "state": {"id": IN_PROGRESS, "name": "In Progress", "group": "started"}, + }, + "activity": {"field": "state", "new_value": IN_PROGRESS, "old_value": BACKLOG}, + }) + assert resp.status_code == 200 + mock_enqueue.assert_called_once() + # task_desc is the 3rd positional arg of enqueue_job(agent, repo, task_desc, ...) + task_desc = mock_enqueue.call_args.args[2] + assert "Description:" in task_desc + # the actual description body (not just the Title) is in the file + assert "REAL BUSINESS REQUEST BODY" in task_desc + assert "results map" in task_desc + + +# --------------------------------------------------------------------------- # +# BUG B: name fetched from Plane API when payload is empty -> slug not untitled +# --------------------------------------------------------------------------- # +@patch("src.webhooks.plane.enqueue_job", return_value=1) +@patch("src.webhooks.plane._create_initial_docs", new_callable=AsyncMock) +@patch("src.webhooks.plane._create_gitea_branch", new_callable=AsyncMock) +@patch("src.plane_sync.fetch_issue_sequence_id", return_value=11) +@patch("src.plane_sync.fetch_issue_fields", + return_value=("GPX upload feature", + "A sufficiently long description so QG-0 passes cleanly.")) +def test_name_fetched_when_payload_empty( + mock_fields, mock_seq, mock_branch, mock_docs, mock_enqueue +): + resp = client.post("/webhook/plane", json={ + "event": "issue", "action": "updated", + "data": { + "id": "taskB", + # NO name, NO description in the payload (Plane status-change shape) + "project": ENDURO_PLANE_ID, + "state": {"id": IN_PROGRESS, "name": "In Progress", "group": "started"}, + }, + "activity": {"field": "state", "new_value": IN_PROGRESS, "old_value": BACKLOG}, + }) + assert resp.status_code == 200 + mock_fields.assert_called_once() + row = _task("taskB") + assert row is not None + branch = row["branch"] + # slug derived from the fetched name -> "gpx-upload-feature", NOT untitled + assert "untitled" not in branch + assert "gpx-upload-feature" in branch + # Title in the analyst task file is the fetched name, not "untitled" + task_desc = mock_enqueue.call_args.args[2] + assert "Title: GPX upload feature" in task_desc