diff --git a/loop.py b/loop.py index 48cbd68..1c10d9c 100644 --- a/loop.py +++ b/loop.py @@ -213,6 +213,70 @@ def _pr_body(result: str, feature: dict) -> str: return body[:4000] +# ── source-issue → PR "Fixes #N" line (pure metadata; the coder never touches it) ─ +# At PR-open the loop stamps the ORIGINATING issue onto the generated body itself. +# The source issue is either an explicit ``source_issue`` field or the FIRST GitHub +# issue URL in the feature's text. When that issue lives in the PR's OWN target repo, +# ``Fixes #N`` (GitHub's repo-scoped closing keyword) auto-closes it on merge; a +# cross-repo issue can't be closed by a bare ``#N`` there, so it gets a plain +# ``Refs `` link instead. One line of pure metadata — no coder round-trip. +_ISSUE_URL_RE = re.compile(r"https://github\.com/([^/\s]+/[^/\s]+)/issues/(\d+)") +# GitHub's issue-closing keywords (close/fix/resolve + their conjugations). +_CLOSING_KW = r"(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)" + + +def _source_issue(feature: dict) -> tuple[str, int] | None: + """The issue this feature's PR should reference: ``(slug, n)`` — ``slug`` is the + issue's ``owner/repo`` (``""`` when the source is a bare number ⇒ same repo) and + ``n`` the issue number — or ``None`` when the feature names no source issue. + + Precedence: an explicit ``source_issue`` field (a full URL, ``owner/repo#n``, or a + bare ``#n``/``n``) wins; otherwise the FIRST GitHub issue URL in the feature text.""" + raw = str(feature.get("source_issue") or "").strip() + if raw: + m = _ISSUE_URL_RE.search(raw) + if m: + return (m.group(1), int(m.group(2))) + m = re.fullmatch(r"([^/\s]+/[^/\s]+)#(\d+)", raw) + if m: + return (m.group(1), int(m.group(2))) + m = re.fullmatch(r"#?(\d+)", raw) + if m: + return ("", int(m.group(1))) + return None + text = "\n".join( + str(feature.get(k) or "") for k in ("description", "spec", "design", "acceptance_criteria", "title") + ) + m = _ISSUE_URL_RE.search(text) + return (m.group(1), int(m.group(2))) if m else None + + +def _inject_source_issue_line(body: str, issue_slug: str, n: int, target_repo: str) -> str: + """Append the source-issue reference to ``body`` (idempotently). + + Same-repo (the issue's ``owner/repo`` equals the PR's ``target_repo``, or the + source was a bare number) ⇒ ``Fixes #n`` (auto-closes on merge); cross-repo ⇒ + ``Refs `` — a bare ``#n`` can't refer to another repo's issue, and an + unresolvable ``target_repo`` (``repo_slug`` failed open) degrades to the safe + ``Refs`` link rather than a possibly-wrong ``Fixes``. + + Suppression is scope-aware: a full-URL reference to THIS issue (``\\b``-bounded, so + ``issues/12`` never matches ``issues/123``) suppresses in either case; the + ``Fixes/Closes #n`` shorthand only suppresses SAME-repo — cross-repo it cannot + name this issue, so it must NOT block the ``Refs`` line.""" + url = f"https://github.com/{issue_slug}/issues/{n}" if issue_slug else "" + same_repo = (not issue_slug) or (bool(target_repo) and issue_slug.lower() == target_repo.lower()) + url_present = bool(url) and re.search(rf"https://github\.com/{re.escape(issue_slug)}/issues/{n}\b", body) + if same_repo: + shorthand = re.search(rf"{_CLOSING_KW}\s+#{n}\b", body, re.I) + if shorthand or url_present: + return body + return f"{body.rstrip()}\n\nFixes #{n}" + if url_present: + return body + return f"{body.rstrip()}\n\nRefs {url}" + + _MAX_MODE_JUDGE_SYS = ( "You are a strict code reviewer choosing the best of several diffs for the same " "task. Pick the one that most completely and correctly satisfies the acceptance " @@ -597,7 +661,8 @@ async def _salvage_verified_candidate(self, store, fid: str) -> bool: self._clear_verified(store, fid) return False title = f"feat: {f.get('title') or fid}" - pr_url = await worktree.open_pr(wt, branch, base=base, title=title, body=_pr_body("", f)) + body = await self._with_source_issue_ref(f, wt, _pr_body("", f)) + pr_url = await worktree.open_pr(wt, branch, base=base, title=title, body=body) store.open_review(fid, pr_url=pr_url) self._clear_verified(store, fid) log.info("[project_board] %s salvaged the verified candidate → %s (no re-solve)", fid, pr_url) @@ -1133,7 +1198,8 @@ async def _drive(self, feature: dict): fid, n, ) - pr_url = await worktree.open_pr(wt, branch, base=base, title=title, body=_pr_body(result, feature)) + body = await self._with_source_issue_ref(feature, wt, _pr_body(result, feature)) + pr_url = await worktree.open_pr(wt, branch, base=base, title=title, body=body) except (worktree.NoChangesError, worktree.WorktreeError) as exc: policy = classify(str(exc)) # A capability failure = the coder didn't deliver (no diff / dispatch @@ -1207,6 +1273,19 @@ async def _drive(self, feature: dict): await worktree.remove_worktree(repo, wt, branch or "") self._inflight.pop(fid, None) + async def _with_source_issue_ref(self, feature: dict, wt: str, body: str) -> str: + """Stamp the feature's source issue onto the PR body — ``Fixes #n`` when the + issue is in the PR's own target repo, ``Refs `` cross-repo. No source + issue ⇒ body unchanged (and no ``gh`` round-trip). ``worktree.repo_slug`` fails + open, so an unresolvable target repo degrades to a ``Refs`` link — never a + wrong ``Fixes`` and never a blocked PR.""" + parsed = _source_issue(feature) + if not parsed: + return body + issue_slug, n = parsed + target_repo = await worktree.repo_slug(cwd=wt) + return _inject_source_issue_line(body, issue_slug, n, target_repo) + async def _request_review(self, fid: str, pr_url: str): """Hand the PR to the reviewer (an a2a delegate, e.g. quinn). Best-effort: a review-dispatch failure doesn't block the feature — CI + the merge diff --git a/tests/test_loop.py b/tests/test_loop.py index 7e08072..23965e7 100644 --- a/tests/test_loop.py +++ b/tests/test_loop.py @@ -13,7 +13,14 @@ import asyncio from project_board import worktree -from project_board.loop import BoardLoop, _ci_failure_reason, _pr_body, _resolve_gate_cmd +from project_board.loop import ( + BoardLoop, + _ci_failure_reason, + _inject_source_issue_line, + _pr_body, + _resolve_gate_cmd, + _source_issue, +) class FakeLoopStore: @@ -2543,3 +2550,111 @@ def loop_install(): from project_board.loop import _PNPM_INSTALL return _PNPM_INSTALL + + +# ── source-issue → PR "Fixes #N" line (#93) ───────────────────────────────────── + + +def test_source_issue_prefers_field_then_scans_description(): + """An explicit `source_issue` field wins (full URL / owner-repo#n / bare number); + otherwise the FIRST GitHub issue URL in the feature text is used.""" + assert _source_issue({"source_issue": "https://github.com/acme/widgets/issues/8"}) == ("acme/widgets", 8) + assert _source_issue({"source_issue": "other/repo#12"}) == ("other/repo", 12) + assert _source_issue({"source_issue": "#4"}) == ("", 4) + assert _source_issue({"source_issue": "4"}) == ("", 4) + # No field → the FIRST issue URL in the text (spec here) wins over the later one. + feat = {"spec": "per https://github.com/acme/widgets/issues/1 then https://github.com/acme/widgets/issues/2"} + assert _source_issue(feat) == ("acme/widgets", 1) + assert _source_issue({"spec": "no issue linked here"}) is None + + +def test_inject_same_repo_appends_fixes_and_is_idempotent(): + body = "## Summary\n\n- did the thing" + out = _inject_source_issue_line(body, "acme/widgets", 7, "acme/widgets") + assert out == body + "\n\nFixes #7" + # A same-repo closing reference already present suppresses (no duplicate). + assert _inject_source_issue_line(out, "acme/widgets", 7, "acme/widgets") == out + assert _inject_source_issue_line("Closes #7 already", "acme/widgets", 7, "acme/widgets") == "Closes #7 already" + + +def test_inject_cross_repo_appends_refs_link(): + body = "## Summary\n\n- did it" + out = _inject_source_issue_line(body, "other/repo", 9, "acme/widgets") + assert out == body + "\n\nRefs https://github.com/other/repo/issues/9" + + +def test_inject_url_dedup_is_word_bounded(): + """FIX-NOW item 1: the URL suppression must be \\b-bounded — issues/12 is a substring + of issues/123, so a body that only references #123 must NOT suppress a #12 line.""" + body = "See https://github.com/acme/widgets/issues/123 for context." + out = _inject_source_issue_line(body, "acme/widgets", 12, "acme/widgets") + assert out.endswith("Fixes #12") # the 123 URL is not a match for #12 + # The EXACT-issue URL already present does suppress (a real duplicate reference). + exact = "Already linked https://github.com/acme/widgets/issues/12 here." + assert _inject_source_issue_line(exact, "acme/widgets", 12, "acme/widgets") == exact + + +def test_inject_cross_repo_shorthand_does_not_suppress(): + """FIX-NOW item 2: a bare `Fixes #42` can't name another repo's issue, so cross-repo + it must NOT suppress the Refs line — only a full-URL match may.""" + body = "Unrelated: Fixes #42 in this repo." + out = _inject_source_issue_line(body, "other/repo", 42, "acme/widgets") + assert out.endswith("Refs https://github.com/other/repo/issues/42") + # The cross-repo URL already present DOES suppress (no duplicate link). + linked = "Refs https://github.com/other/repo/issues/42 already." + assert _inject_source_issue_line(linked, "other/repo", 42, "acme/widgets") == linked + + +def test_inject_bare_number_is_same_repo_even_when_target_unknown(): + # A bare number (slug "") is same-repo by construction — Fixes #n even if the target + # repo couldn't be resolved (repo_slug failed open); never a Refs to an empty slug. + assert _inject_source_issue_line("body", "", 5, "") == "body\n\nFixes #5" + + +def test_inject_unknown_target_degrades_to_refs_for_a_slugged_issue(): + # repo_slug failed open (target ""): a slugged issue can't be confirmed same-repo, so + # the safe degrade is a Refs link, never a possibly-wrong Fixes that auto-closes here. + out = _inject_source_issue_line("body", "acme/widgets", 3, "") + assert out == "body\n\nRefs https://github.com/acme/widgets/issues/3" + + +async def test_repo_slug_fails_open_on_gh_error(monkeypatch): + """FIX-NOW item 3: worktree.repo_slug must fail OPEN — a raising _gh (WorktreeError / + timeout) yields "" rather than propagating; a non-zero rc also yields "".""" + + async def _boom(*args, cwd, timeout=60): + raise worktree.WorktreeError("gh repo view timed out after 60s") + + monkeypatch.setattr(worktree, "_gh", _boom) + assert await worktree.repo_slug(cwd="/repo") == "" + + async def _rc1(*args, cwd, timeout=60): + return (1, "", "not a gh repo") + + monkeypatch.setattr(worktree, "_gh", _rc1) + assert await worktree.repo_slug(cwd="/repo") == "" + + async def _ok(*args, cwd, timeout=60): + return (0, "acme/widgets\n", "") + + monkeypatch.setattr(worktree, "_gh", _ok) + assert await worktree.repo_slug(cwd="/repo") == "acme/widgets" + + +async def test_drive_injects_fixes_line_into_the_pr_body(monkeypatch): + """End-to-end: a feature carrying a same-repo source issue gets a `Fixes #n` line + appended to the body the loop hands to open_pr (the coder stays out of the loop).""" + bodies = [] + + async def _open_pr(wt, branch, *, base, title, body): + bodies.append(body) + return "https://example/pr/1" + + async def _slug(*, cwd): + return "acme/widgets" + + monkeypatch.setattr(worktree, "repo_slug", _slug) + feature = {**FEATURE, "source_issue": "https://github.com/acme/widgets/issues/7"} + _loop, store = await _drive_with(monkeypatch, open_pr=_open_pr, feature=feature) + assert ("open_review", "bd-1", "https://example/pr/1") in store.calls + assert bodies and bodies[0].endswith("Fixes #7") diff --git a/worktree.py b/worktree.py index ee05bd0..d164c11 100644 --- a/worktree.py +++ b/worktree.py @@ -492,3 +492,18 @@ async def pr_url_for_branch(branch: str, *, cwd: str = ".") -> str: adopting → in_review) from one that needs a fresh rebuild.""" rc, out, _err = await _gh("pr", "view", branch, "--json", "url", "--jq", ".url", cwd=cwd) return out.strip() if rc == 0 else "" + + +async def repo_slug(*, cwd: str = ".") -> str: + """The ``owner/name`` slug of the checkout's default GitHub repo — the repo a PR + opened from here TARGETS — or ``""`` when it can't be resolved. + + Fails OPEN: a ``gh`` non-zero exit OR a ``WorktreeError`` (the timeout ``_gh`` + raises) returns ``""`` instead of propagating, so a caller (e.g. the PR-body + source-issue stamp) that can't learn the target repo simply degrades rather than + blocking the PR. This never raises into the loop.""" + try: + rc, out, _err = await _gh("repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner", cwd=cwd) + except WorktreeError: + return "" + return out.strip() if rc == 0 else ""