diff --git a/.env.example b/.env.example index 80ed38a..0820341 100644 --- a/.env.example +++ b/.env.example @@ -28,6 +28,13 @@ GH_PROJECT_TARGET_FIELD=Target # agent:in-progress, agent:in-review, and agent:ready are always excluded (they drive lane movement). LABEL_SYNC_IGNORE= +# Optional: max character length of an AgilePlace card description (rendered HTML) before the sync +# truncates it (appending a marker) rather than risking a write-time error. AgilePlace's own limit is +# undocumented, so this defaults to a conservative value below every documented LeanKit/AgilePlace +# field limit we could find. Must be a positive integer; anything else falls back to the default with +# a WARN. +AP_DESCRIPTION_MAX_LENGTH= + # Optional: map GitHub stages -> YOUR board's lanes when lane titles don't self-describe. Format: # ';'-separated "Stage=Lane" entries; '|' lists multiple lanes per stage (FIRST = move-to target, ALL = # "already in that stage", so the sync won't shuffle a card between equivalent lanes). Stages are: diff --git a/agileplace_description.py b/agileplace_description.py new file mode 100644 index 0000000..17902eb --- /dev/null +++ b/agileplace_description.py @@ -0,0 +1,32 @@ +"""AgilePlace card-description read/write helpers (issue #65 Task 1/7). + +Split out of agileplace.py -- rather than growing that module further -- because it was already at +the project's 800-line file cap on this branch's base (code.md: "never add to a file already over +budget -- extract first"). Depends on agileplace.get_card for the lazy-refetch fallback in +card_description(); agileplace.py has no dependency back on this module, so there is no cycle. +""" +from __future__ import annotations + +import agileplace + + +def card_description(cfg: dict, card: dict) -> str: + """The card's description text, normalized to "" for None/absent. list_cards() never returns + `description` (no field-selection params sent, confirmed against the live board), so a card + reaching this function via the board snapshot is almost always missing the key entirely -- that + (and ONLY that) triggers one lazy get_card refetch. A card that DOES carry the key -- including + description="" -- takes the zero-I/O path: an explicit empty string is a real, current + description, not "unknown", and must never be treated as a reason to hit the network.""" + if "description" in card: + return card["description"] or "" + fresh = agileplace.get_card(cfg, card["id"]) + return fresh.get("description") or "" + + +def op_description(html: str) -> dict: + """A single unconditional replace of the card's description with `html` (already rendered by + richtext.markdown_to_leankit_html and, when oversized, truncated by + description_sync._truncate_for_agileplace). No validation here -- description_sync owns the + decision of *whether* to write; this is purely the op-builder, matching agileplace.op_custom_id's + shape.""" + return {"op": "replace", "path": "/description", "value": html} diff --git a/config.py b/config.py index a091ab9..a6566ea 100644 --- a/config.py +++ b/config.py @@ -24,6 +24,12 @@ # over-abstraction for no benefit. _ENV_LOADER_BLOCKLIST = frozenset({"GH_REPO", "GH_HOST"}) +# AgilePlace's `description` field length limit is undocumented (VALIDATE LIVE -- see +# API-VALIDATION.md); this is a conservative ceiling picked to stay well under every publicly +# documented LeanKit/AgilePlace card-field limit we could find. description_sync truncates (with +# TRUNCATION_MARKER appended) rather than risking a write-time 4xx from an oversized description. +DEFAULT_AP_DESCRIPTION_MAX_LENGTH = 20000 + def load_env_file() -> None: """Load KEY=VALUE lines from ./.env; real environment variables win over it. GH_REPO/GH_HOST are @@ -64,6 +70,26 @@ def parse_stage_lane_map(raw: str) -> dict[str, list[str]]: return mapping +def _parse_ap_description_max_length(raw: str | None) -> int: + """AP_DESCRIPTION_MAX_LENGTH from .env/environment, falling back to + DEFAULT_AP_DESCRIPTION_MAX_LENGTH (with one WARN) on anything that isn't a positive int -- a + malformed override must never silently disable truncation (e.g. becoming 0/negative) or crash + env_config() outright.""" + if raw is None or not raw.strip(): + return DEFAULT_AP_DESCRIPTION_MAX_LENGTH + try: + value = int(raw.strip()) + except ValueError: + print(f"WARN AP_DESCRIPTION_MAX_LENGTH={raw!r} is not an integer -- using default " + f"{DEFAULT_AP_DESCRIPTION_MAX_LENGTH}") + return DEFAULT_AP_DESCRIPTION_MAX_LENGTH + if value <= 0: + print(f"WARN AP_DESCRIPTION_MAX_LENGTH={raw!r} must be a positive integer -- using default " + f"{DEFAULT_AP_DESCRIPTION_MAX_LENGTH}") + return DEFAULT_AP_DESCRIPTION_MAX_LENGTH + return value + + def env_config() -> dict: """token/host/board_id are None when absent (offline dry run). target_repo_path is the local clone every `gh` call runs against.""" @@ -78,6 +104,8 @@ def env_config() -> dict: "target_repo_path": Path(target).expanduser() if target else None, "label_sync_ignore": frozenset(DEFAULT_IGNORE) | frozenset(extra), "stage_lane_map": parse_stage_lane_map(os.environ.get("STAGE_LANE_MAP", "")), + "ap_description_max_length": _parse_ap_description_max_length( + os.environ.get("AP_DESCRIPTION_MAX_LENGTH")), "gh_project": { "owner": os.environ.get("GH_PROJECT_OWNER") or None, "number": os.environ.get("GH_PROJECT_NUMBER") or None, diff --git a/description_sync.py b/description_sync.py new file mode 100644 index 0000000..fac0538 --- /dev/null +++ b/description_sync.py @@ -0,0 +1,240 @@ +"""Pure GH<->AgilePlace description merge for issue #65. No I/O -- exhaustively unit-tested. + +Two canonicalization helpers put each side's raw text into a stable, comparable "canonical +Markdown" form. richtext's HTML<->Markdown translation absorbs cosmetic round-trip variance (e.g. +`*bold*` vs `_bold_`-equivalent AgilePlace HTML) so it never registers as a spurious edit -- +echo prevention falls out of comparing canonicalized values, never raw ones. `resolve_description` +is the 3-way merge itself: `base`/`ap_written_base` are the last-synced canonical values persisted +in .sync-state.json (see sync.py's issues_state); `gh_canonical`/`ap_canonical` are this run's +freshly canonicalized GitHub issue body and AgilePlace card description. + +Conflict policy is warn-and-skip (issue #65's brainstormed decision, deliberately breaking the +AgilePlace-wins-dates precedent set by sync_dates): when BOTH sides changed since their own +reference point AND landed on genuinely different values, neither side is overwritten -- prose +edits are too costly to silently destroy. When both sides changed but converged on the identical +value (e.g. two people independently typing the same fix), that is not a conflict: there is +nothing to warn about and nothing left to write. +""" +from __future__ import annotations + +from collections.abc import Callable +from typing import NamedTuple + +import agileplace_description +import ghkit +import richtext +from stages import issue_custom_id + +# Appended to a truncated AgilePlace description so a reader knows the full text lives on GitHub. +# Exact text pinned by the issue #65 design doc -- config.py's DEFAULT_AP_DESCRIPTION_MAX_LENGTH +# docstring references this same constant by name. +TRUNCATION_MARKER = "…[truncated by sync — full text on GitHub]" + + +class DescriptionResolution(NamedTuple): + """Result of one resolve_description() call. + + Invariant: `conflict` is True iff `write_gh` and `write_ap` are both False AND `warning` is not + None AND `merged == (base or "")`. A genuine conflict never touches either side and always + reports the OLD agreed value, never a partial or guessed merge. (Steady state also has both + write flags False and `merged == (base or "")` -- `warning is None` is what tells the two + apart.) + """ + merged: str + write_gh: bool + write_ap: bool + conflict: bool + warning: str | None + + +def resolve_description(base: str | None, ap_written_base: str | None, gh_canonical: str, + ap_canonical: str) -> DescriptionResolution: + """3-way merge of a GitHub issue body against an AgilePlace card description, both already + canonicalized (see _canonicalize_gh_body / _canonicalize_ap_description below). Pure, total, + no I/O. + + `base` is the full agreed canonical Markdown as of the last successful sync (None on a + never-synced issue). `ap_written_base` is the canonical of what was actually WRITTEN to the + card last time -- the two differ only when a prior run truncated an oversized description, so + the shorter, truncated text left on the card is compared against its own prior truncated form, + never against the full untruncated base. Without that split, a card still carrying last run's + truncated text would look AP-side-edited on every subsequent run, forever. + + Change is detected independently per side against its own reference point; `None` normalizes + to "" on both sides. Five outcomes: + - neither side changed -> no write; `merged` is the unchanged base (steady state, including + the truncated-steady-state where the card still carries last run's truncated text). + - only the GH side changed -> propagates to AgilePlace; `merged` is the GH canonical text. + - only the AP side changed, AND `ap_written_base` already equalled `base` (the card carried + the FULL text, never a truncated stand-in) -> propagates to GitHub; `merged` is the AP + canonical text. + - only the AP side changed, but `ap_written_base` diverges from `base` (the card has only + ever carried a truncated stand-in for the full body) -> treated as unresolvable: the edit + touches only the visible truncated portion, never the untruncated tail that lives solely + in `base`, so promoting it to GitHub would silently destroy that tail. Degrades to the same + conflict outcome as both-sides-changed: neither side is written, `merged` stays the old + base, and `warning` names the issue for a human to reconcile by hand. + - both changed but landed on the SAME value -> no conflict, no write (independently + converged); `merged` becomes that shared value so the base can advance to it. + - both changed to DIFFERENT values -> conflict: neither side is written, `merged` stays the + old base, and `warning` names the conflict for a human to reconcile by hand. + """ + base_norm = base or "" + ap_written_norm = ap_written_base or "" + gh_changed = gh_canonical != base_norm + ap_changed = ap_canonical != ap_written_norm + + if not gh_changed and not ap_changed: + return DescriptionResolution(base_norm, False, False, False, None) + if gh_changed and not ap_changed: + return DescriptionResolution(gh_canonical, False, True, False, None) + if ap_changed and not gh_changed: + if ap_written_norm != base_norm: + # ap_written_base diverges from base ONLY when a prior run truncated the full body for + # the card (see _truncate_for_agileplace) -- the card has never carried anything but a + # truncated stand-in. An edit on top of that stub only ever touches the visible + # truncated portion, never the untruncated tail that lives solely in `base`. Blindly + # promoting ap_canonical to `merged` here would push that short stub to GitHub and + # permanently destroy the lost tail with no warning. Degrade to the same warn-and-skip + # conflict policy as a genuine both-sides conflict instead. + warning = ("description conflict: the AgilePlace card description changed, but the " + "card holds only a truncated stand-in for the full GitHub issue body -- the " + "edit cannot be safely merged back without risking loss of the untruncated " + "tail -- leaving both sides untouched until a human reconciles them by hand") + return DescriptionResolution(base_norm, False, False, True, warning) + return DescriptionResolution(ap_canonical, True, False, False, None) + if gh_canonical == ap_canonical: + return DescriptionResolution(gh_canonical, False, False, False, None) + warning = ("description conflict: both the GitHub issue body and the AgilePlace card " + "description changed since the last sync and now disagree -- leaving both sides " + "untouched until a human reconciles them by hand") + return DescriptionResolution(base_norm, False, False, True, warning) + + +def _canonicalize_gh_body(body: str | None) -> str: + """Canonical Markdown for a GitHub issue body: a genuine md->html->md round trip absorbs + cosmetic Markdown variance (e.g. equivalent emphasis-marker choices) that would otherwise + register as a spurious edit every run. None/missing normalizes to "". + + Self-composition-idempotent: canonical(canonical(x)) == canonical(x). A second round trip + starts from output that is already a fixed point of richtext's md->html->md translation, so + re-canonicalizing it reproduces the same text.""" + return richtext.leankit_html_to_markdown(richtext.markdown_to_leankit_html(body or "")) + + +def _canonicalize_ap_description(description: str | None) -> str: + """Canonical Markdown for an AgilePlace card description, which AgilePlace stores as HTML: one + html->md translation. None/missing normalizes to "". + + NOT self-composition-idempotent: calling this twice in a row would feed its own Markdown + output back in as though it were HTML -- a type mismatch, not a no-op, since richtext's HTML-> + Markdown walker parses stray Markdown punctuation as literal text rather than recognizing it. + The real invariant is a round trip THROUGH HTML: + `_canonicalize_ap_description(html) == + _canonicalize_ap_description(markdown_to_leankit_html(_canonicalize_ap_description(html)))` + -- re-rendering the canonical Markdown back to HTML and canonicalizing THAT reproduces the + same canonical Markdown.""" + return richtext.leankit_html_to_markdown(description or "") + + +def _snap_to_whitespace_boundary(markdown: str, index: int) -> int: + """Snap a candidate cut `index` down to the nearest whitespace boundary at or before it, so a + truncation cut never splits a word. Clamped to [0, len(markdown)] first, so callers can pass + any binary-search midpoint without pre-validating it. Returns 0 when no whitespace precedes + `index` (e.g. one giant unbroken token) -- a zero-length prefix is a valid degenerate result, + never a negative slice.""" + index = max(0, min(index, len(markdown))) + while index > 0 and not markdown[index - 1].isspace(): + index -= 1 + return index + + +def _truncate_for_agileplace(markdown: str, max_length: int) -> tuple[str, bool]: + """Render `markdown` to AgilePlace HTML, truncating at a clean word boundary with + TRUNCATION_MARKER appended if the render exceeds `max_length` characters. Returns + (html, was_truncated). + + Binary-searches the markdown cut-length against rendered-HTML length (each candidate cut + snapped to its preceding whitespace boundary, re-rendered once, with TRUNCATION_MARKER's + length folded into the same length check) instead of shrinking one word at a time -- O(log n) + renders instead of O(n). That distinction is the difference between milliseconds and 50+ + seconds on a realistically large GH issue body (up to 65,536 chars): a prior per-word shrink + loop re-rendered the whole prefix on every single word removed. + + Never negative-length slices (see _snap_to_whitespace_boundary). A max_length too small to fit + even TRUNCATION_MARKER degrades to a marker-only result rather than looping forever or raising + -- the search floor is always a zero-length markdown prefix, which still renders cleanly.""" + full_html = richtext.markdown_to_leankit_html(markdown) + if len(full_html) <= max_length: + return full_html, False + + lo, hi = 0, len(markdown) + best_cut = 0 + while lo <= hi: + mid = (lo + hi) // 2 + cut = _snap_to_whitespace_boundary(markdown, mid) + candidate_len = len(richtext.markdown_to_leankit_html(markdown[:cut])) + len(TRUNCATION_MARKER) + if candidate_len <= max_length: + best_cut = cut + lo = mid + 1 + else: + hi = mid - 1 + + final_html = richtext.markdown_to_leankit_html(markdown[:best_cut]) + TRUNCATION_MARKER + return final_html, True + + +def sync_description(cfg: dict, apply: bool, issue: dict, card: dict, issues_state: dict, + queue: Callable) -> None: + """Wire resolve_description's pure merge into GitHub/AgilePlace writes, with a coupled + base-advance gate mirroring sync_dates' (issue #6) merge-base contract exactly: `desc_base` and + `desc_ap_written` only ever advance TOGETHER, and only when `apply` is True AND the GitHub-side + write is confirmed (`gh_write_ok`). `gh_write_ok` defaults to True whenever no GitHub write was + needed at all (write_gh is False) -- there is nothing to have failed, same as sync_dates. + + The AgilePlace-side queue write is unconditional, also like sync_dates: it fires whenever + write_ap is True regardless of apply or gh_write_ok, and its eventual success/failure at PATCH- + flush time is not fed back into this gate -- a poisoned card's flush skip already holds the + whole run's state per sync.py's own contract (see sync.py's "poisoned card(s) this run" branch). + A write that is skipped, a dry run, or a failed GitHub write must never advance either field -- + and the two fields never advance independently of one another. + + A dry-run-only planned card (``card["_planOnly"]``) exists only as a synthetic snapshot for + continuing this one dry run -- it has no server-side identity yet, so it never carries a + 'description' key. agileplace_description.card_description()'s lazy get_card() fallback would + otherwise issue a live GET for an id that was never created on the server. Same convention sync.py's + dependency-sync loop already uses for plan-only cards ("a fresh card has no server-side + dependencies; never read a plan-only id") -- a fresh card likewise has no server-side + description yet, so the AgilePlace side is treated as "" without any network read. + """ + url = issue["url"] + prev = issues_state[url] + key = issue_custom_id(issue) + + gh_canonical = _canonicalize_gh_body(issue.get("body")) + ap_description = ("" if card.get("_planOnly") + else agileplace_description.card_description(cfg, card)) + ap_canonical = _canonicalize_ap_description(ap_description) + result = resolve_description(prev.get("desc_base"), prev.get("desc_ap_written"), + gh_canonical, ap_canonical) + + if result.conflict: + print(f"WARN [{key}] {result.warning}") + return + + gh_write_ok = True + if result.write_gh: + gh_write_ok = ghkit.edit_issue_body(cfg, apply, issue["number"], result.merged) + + new_ap_written = ap_canonical + if result.write_ap: + html, _ = _truncate_for_agileplace(result.merged, cfg["ap_description_max_length"]) + queue(card, [agileplace_description.op_description(html)], "description") + new_ap_written = _canonicalize_ap_description(html) + + if result.write_gh or result.write_ap: + print(f"desc [{key}] gh={result.write_gh} ap={result.write_ap}") + + if apply and gh_write_ok: + prev["desc_base"] = result.merged + prev["desc_ap_written"] = new_ap_written diff --git a/ghkit.py b/ghkit.py index 43c224e..d629fb3 100644 --- a/ghkit.py +++ b/ghkit.py @@ -115,7 +115,7 @@ def list_issues(cfg: dict) -> list[dict]: the card-creation path. """ out = run(cfg, ["issue", "list", "--state", "all", "--limit", "1000", "--json", - "number,title,state,stateReason,labels,milestone,assignees,url"]) + "number,title,state,stateReason,labels,milestone,assignees,url,body"]) issues = json.loads(out.stdout or "[]") normalized = [] for i in issues: @@ -129,6 +129,7 @@ def list_issues(cfg: dict) -> list[dict]: "milestone": ms.get("title") or None, "assignees": [a.get("login") for a in i.get("assignees", [])], "url": i.get("url", ""), + "body": i.get("body") or "", # description_sync's GitHub-side canonicalization input "has_open_pr": False, # populated by open_pr_issue_numbers() }) return normalized @@ -329,6 +330,33 @@ def set_milestone(cfg: dict, apply: bool, number: int, title: str | None) -> Non print(f"DRY gh issue edit {number} --remove-milestone") +def edit_issue_body(cfg: dict, apply: bool, number: int, body: str) -> bool: + """Set an issue's body via `gh issue edit --body-file -`, through the dry-run gate. The body is + piped through run()'s `input=` stdin passthrough (same idiom as create_issue's --body-file -), + never interpolated into argv, so a description containing shell metacharacters or gh-flag-like + text can't be misparsed. + + Returns True only when the write actually happened (apply=True and gh succeeded) and False for a + dry run -- description_sync.sync_description gates its base-advance on this exact boolean + (gh_write_ok), so a dry run must never report success. Any CalledProcessError/TimeoutExpired from + run() propagates uncaught, matching create_issue/edit_label's own apply=True behavior -- a failed + write must not be swallowed into a false "it worked". + + Validates `number`/`body` at this boundary, before either the dry-run print or a live run() call + -- an invalid number would otherwise reach `gh issue edit ` and fail opaquely, and a + non-string body would crash run()'s stdin pipe with a confusing TypeError.""" + if not isinstance(number, int) or isinstance(number, bool) or number < 1: + raise ValueError(f"edit_issue_body: number must be a positive int, got {number!r}") + if not isinstance(body, str): + raise ValueError(f"edit_issue_body: body must be a str, got {type(body).__name__}") + if not apply: + print(f"DRY gh issue edit {number} --body-file -") + return False + run(cfg, ["issue", "edit", str(number), "--body-file", "-"], input=body) + print(f"gh issue {number} body updated") + return True + + def _issue_number_from_url(url: str) -> int: """The trailing /issues/{n} segment of a GitHub issue URL, as an int. `gh issue create`'s stdout contract is exactly one bare URL line; a malformed one is a genuine unrecovered failure, diff --git a/smoke.py b/smoke.py index 20ed1e8..9e9f5a8 100644 --- a/smoke.py +++ b/smoke.py @@ -3,9 +3,9 @@ Reads .env exactly like sync.py, previews the target board (title, lanes, existing cards), and only after an explicit confirmation creates two clearly-marked throwaway cards, exercises every write shape the sync uses -- card create, versioned PATCH tag add/remove, externalLink add on a bare card, -connect/disconnect children, and a deliberately stale-version PATCH that MUST be rejected -- then -deletes both cards and confirms they are gone. The steps mirror the ``[live-check]`` items in -API-VALIDATION.md so one confirmed run retires them. +connect/disconnect children, description write + length probe (issue #65), and a deliberately +stale-version PATCH that MUST be rejected -- then deletes both cards and confirms they are gone. The +steps mirror the ``[live-check]`` items in API-VALIDATION.md so one confirmed run retires them. GitHub is never touched (dry run already exercises every gh read live), and the sync state file is never read or written. Every server rejection is printed with its full, untruncated response body so @@ -19,6 +19,7 @@ import sys import agileplace +import agileplace_description from config import env_config PARENT_TITLE = "SMOKE parent (safe to delete)" @@ -280,6 +281,48 @@ def _check_dependencies(cfg: dict, parent_id: str, child_id: str, results: list) f"incoming read back: {sorted(incoming) if incoming is not None else 'unavailable'}")) +def _check_description(cfg: dict, parent_id: str, results: list) -> None: + """Steps 14-15: agileplace_description.op_description's exact write shape through the same versioned-PATCH + path (patch_card) the sync itself uses (description_sync.sync_description), then a + fact-finding probe of the configured ap_description_max_length against the live server (the + issue #65 design doc's "max-length probe": cross-check config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + -- or its .env override -- against what the real API actually accepts). + + NOTE -- untested replace-vs-remove uncertainty (issue #65 design decision #7): op_description + always issues an RFC-6902 "replace", including when description_sync writes "" to clear a + card's description. Other fields that need clearing (planned dates, blocked-state) use a + dedicated "remove" op instead of a replace-to-null/empty -- see steps 5-6's live-validated + contract above -- but whether AgilePlace's PATCH endpoint honors a replace-to-"" the same way + for /description has never been checked against the real API. This step only validates a + non-empty replace round-trips correctly; it deliberately does NOT exercise clearing to "", so + that uncertainty stays open (tracked in the design doc) until a live run confirms one way or + the other.""" + _step(14, "description write via op_description through patch_card") + fresh = agileplace.get_card(cfg, parent_id) + html = "

SMOKE description write

" + agileplace.patch_card(cfg, True, fresh, [agileplace_description.op_description(html)]) + readback = agileplace_description.card_description(cfg, agileplace.get_card(cfg, parent_id)) + results.append(("description write round-trip (op_description via patch_card)", + readback == html, f"description now {readback!r}")) + + _step(15, "description length probe (fact-finding, not pass/fail)") + limit = cfg["ap_description_max_length"] + probe_html = f"

{'x' * limit}

" + fresh = agileplace.get_card(cfg, parent_id) + try: + agileplace.patch_card(cfg, True, fresh, [agileplace_description.op_description(probe_html)]) + except SystemExit as exc: + _print_http_failure(exc) + results.append(("description length probe", None, + f"server REJECTED a {len(probe_html)}-char description " + f"(configured max_length={limit}): {exc}")) + return + readback = agileplace_description.card_description(cfg, agileplace.get_card(cfg, parent_id)) + results.append(("description length probe", None, + f"sent {len(probe_html)} chars, server stored {len(readback)} chars back " + f"(configured max_length={limit})")) + + def _check_stale_patch(cfg: dict, parent_id: str, stale_version: str, results: list) -> None: """Step 13: a stale x-lk-resource-version PATCH must be rejected, not silently applied.""" _step(13, "deliberately stale-version PATCH (the server MUST reject this)") @@ -338,6 +381,7 @@ def _run_checks(cfg: dict, lane_id: str | None, run_id: str, created: list[str], child_id = _check_child_and_link(cfg, lane_id, CHILD_CUSTOM_ID_PREFIX + run_id, created, results) _check_connections(cfg, parent_id, child_id, results) _check_dependencies(cfg, parent_id, child_id, results) + _check_description(cfg, parent_id, results) _check_stale_patch(cfg, parent_id, baseline_version, results) diff --git a/sync.py b/sync.py index 77c6d85..691816d 100644 --- a/sync.py +++ b/sync.py @@ -28,6 +28,7 @@ from card_coherence import (contested_cards, fence_run_indices, filter_poisoned_edges, laneid_op_value, lane_conflict, poisoned_card_ids, same_card) from config import STATE_FILE, env_config +from description_sync import sync_description from metadata_sync import sync_dates, sync_metadata from stages import (epic_key_for_task, is_retired_issue, issue_custom_id, issue_stage, normalize_status, title_key) @@ -679,6 +680,7 @@ def queue(card, ops, note): sync_metadata(cfg, apply, issue, card, cfg["label_sync_ignore"], issues_state, queue) if field_meta: sync_dates(cfg, apply, issue, card, project_items.get(issue["url"]), field_meta, issues_state, queue) + sync_description(cfg, apply, issue, card, issues_state, queue) # 3) parent/child connections (see sync_child_connections for the full contract). poisoned = poisoned_card_ids(card_ops) diff --git a/tests/test_agileplace.py b/tests/test_agileplace.py index cd8f011..074f2ea 100644 --- a/tests/test_agileplace.py +++ b/tests/test_agileplace.py @@ -789,6 +789,11 @@ def fake_api_double_miss(cfg, method, path, body=None, params=None, headers=None patch_card(CFG, True, card, ops) +# op_description / card_description (issue #65 Task 1/7) moved to agileplace_description.py and +# tests/test_agileplace_description.py -- see review finding: keep this file from growing past the +# project's file-size cap by extracting the description read/write surface into its own module. + + def test_connect_children_disconnect_children_create_card_have_no_version_header_logic(): """Non-goal preserved: no version header logic is added to any non-card-PATCH endpoint.""" with patch("agileplace.api", return_value={}) as api_mock: diff --git a/tests/test_agileplace_description.py b/tests/test_agileplace_description.py new file mode 100644 index 0000000..18cf47c --- /dev/null +++ b/tests/test_agileplace_description.py @@ -0,0 +1,72 @@ +"""Unit tests for agileplace_description.py's card-description read/write helpers (issue #65 Task +1/7). No network -- pins the JSON Patch shape and the lazy-refetch fallback contract. Split out of +test_agileplace.py alongside the card_description/op_description extraction (review finding: keep +agileplace.py, and its test file, from growing past the project's file-size cap). Run: pytest -q +""" +import sys +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from agileplace_description import card_description, op_description # noqa: E402 + +CFG = {"token": "t", "host": "h", "board_id": "b1"} + + +def test_op_description_returns_single_replace_op(): + """No validation here (description_sync owns write-vs-not) -- matches op_custom_id's shape.""" + op = op_description("

hello

") + assert op == {"op": "replace", "path": "/description", "value": "

hello

"} + + +def test_card_description_returns_present_description_without_network_call(): + card = {"id": "1", "description": "

hi

"} + with patch("agileplace_description.agileplace.get_card") as get_card_mock: + result = card_description(CFG, card) + get_card_mock.assert_not_called() + assert result == "

hi

" + + +def test_card_description_present_but_empty_string_takes_zero_io_path(): + """A card literally carrying description="" is a real, current empty description -- NOT + 'unknown' -- and must not trigger the lazy get_card fallback (see struct #7 in the design: + every fixture reaching this must either carry the key or explicitly mock get_card).""" + card = {"id": "1", "description": ""} + with patch("agileplace_description.agileplace.get_card") as get_card_mock: + result = card_description(CFG, card) + get_card_mock.assert_not_called() + assert result == "" + + +def test_card_description_present_but_none_normalizes_to_empty_string(): + card = {"id": "1", "description": None} + with patch("agileplace_description.agileplace.get_card") as get_card_mock: + result = card_description(CFG, card) + get_card_mock.assert_not_called() + assert result == "" + + +def test_card_description_missing_key_falls_back_to_lazy_get_card(): + """list_cards() never returns description (no field-selection params sent) -- a card summary + missing the key entirely must trigger exactly one get_card refetch.""" + card = {"id": "77"} + with patch("agileplace_description.agileplace.get_card", + return_value={"id": "77", "description": "

fresh

"}) as get_card_mock: + result = card_description(CFG, card) + get_card_mock.assert_called_once_with(CFG, "77") + assert result == "

fresh

" + + +def test_card_description_lazy_fallback_normalizes_missing_description_to_empty_string(): + card = {"id": "78"} + with patch("agileplace_description.agileplace.get_card", return_value={"id": "78"}): + result = card_description(CFG, card) + assert result == "" + + +def test_card_description_never_mutates_input_card(): + card = {"id": "1", "description": "

hi

"} + before = dict(card) + card_description(CFG, card) + assert card == before diff --git a/tests/test_config_env.py b/tests/test_config_env.py index 893910b..29168c1 100644 --- a/tests/test_config_env.py +++ b/tests/test_config_env.py @@ -86,3 +86,75 @@ def spy_setdefault(key, value): assert "AGILEPLACE_TOKEN" in setdefault_calls # unrelated key still goes through setdefault assert os.environ.get("GH_REPO") == "correct/repo" # untouched, never overwritten assert os.environ.get("GH_HOST") == "correct.example.com" + + +# --- AP_DESCRIPTION_MAX_LENGTH: safe int parse with WARN fallback (issue #65 Task 1) ------------- +# +# description_sync's truncation boundary depends on env_config() always handing back a usable +# positive int here, whatever garbage a .env file or the real environment supplies -- these tests +# pin that env_config() itself never raises and never silently produces a 0/negative ceiling that +# would degrade _truncate_for_agileplace to a marker-only result on every single run. + +def test_default_ap_description_max_length_is_a_positive_int(): + assert isinstance(config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH, int) + assert config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH > 0 + + +def test_env_config_ap_description_max_length_defaults_when_unset(tmp_path, monkeypatch): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") # no .env file present + monkeypatch.delenv("AP_DESCRIPTION_MAX_LENGTH", raising=False) + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + + +def test_env_config_ap_description_max_length_reads_valid_override(tmp_path, monkeypatch): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") + monkeypatch.setenv("AP_DESCRIPTION_MAX_LENGTH", "5000") + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == 5000 + + +def test_env_config_ap_description_max_length_falls_back_and_warns_on_non_int(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") + monkeypatch.setenv("AP_DESCRIPTION_MAX_LENGTH", "not-a-number") + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + out = capsys.readouterr().out + assert "WARN" in out + assert "AP_DESCRIPTION_MAX_LENGTH" in out + + +def test_env_config_ap_description_max_length_falls_back_and_warns_on_zero(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") + monkeypatch.setenv("AP_DESCRIPTION_MAX_LENGTH", "0") + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + assert "WARN" in capsys.readouterr().out + + +def test_env_config_ap_description_max_length_falls_back_and_warns_on_negative(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") + monkeypatch.setenv("AP_DESCRIPTION_MAX_LENGTH", "-1") + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + assert "WARN" in capsys.readouterr().out + + +def test_env_config_ap_description_max_length_treats_blank_as_unset(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(config, "ENV_FILE", tmp_path / ".env") + monkeypatch.setenv("AP_DESCRIPTION_MAX_LENGTH", " ") + + cfg = config.env_config() + + assert cfg["ap_description_max_length"] == config.DEFAULT_AP_DESCRIPTION_MAX_LENGTH + assert capsys.readouterr().out == "" # blank is "unset", not a malformed value -- no WARN noise diff --git a/tests/test_description_sync.py b/tests/test_description_sync.py new file mode 100644 index 0000000..6983690 --- /dev/null +++ b/tests/test_description_sync.py @@ -0,0 +1,425 @@ +"""Unit tests for description_sync.py's pure merge (issue #65). No I/O -- pins the full 10-state +resolve_description matrix plus the two canonicalization helpers' (corrected) idempotence +invariants. Run: pytest -q +""" +from __future__ import annotations + +import math +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import richtext # noqa: E402 +from description_sync import ( # noqa: E402 + TRUNCATION_MARKER, + DescriptionResolution, + _canonicalize_ap_description, + _canonicalize_gh_body, + _truncate_for_agileplace, + resolve_description, + sync_description, +) + +ISSUE_URL = "https://github.com/acme/repo/issues/1" + + +def _issue(body="", url=ISSUE_URL, number=1, title="[T1] widget"): + return {"number": number, "title": title, "url": url, "body": body} + + +def _card(card_id="C1"): + return {"id": card_id} + + +def _cfg(): + return {"ap_description_max_length": 20000} + + +class _Queue: + """Records every queue(card, ops, note) call for assertions -- same spy shape as + test_sync_dates.py's _Queue.""" + def __init__(self): + self.calls = [] + + def __call__(self, card, ops, note): + self.calls.append((card, ops, note)) + + +def _assert_invariant(base: str | None, result: DescriptionResolution) -> None: + """The invariant description_sync.py's own docstring states: conflict==True iff write_gh and + write_ap are both False AND warning is not None AND merged == (base or "").""" + conflict_shape = ( + result.write_gh is False and result.write_ap is False + and result.warning is not None and result.merged == (base or "") + ) + assert result.conflict == conflict_shape + + +# ===================================================================================== +# resolve_description -- the 10 hand-traced states +# ===================================================================================== + +def test_seeding_card_empty_pushes_gh_body_to_agileplace(): + result = resolve_description(None, None, "Hello from GitHub", "") + assert result == DescriptionResolution("Hello from GitHub", False, True, False, None) + _assert_invariant(None, result) + + +def test_seeding_gh_body_empty_pulls_card_description_to_github(): + result = resolve_description(None, None, "", "Hello from AgilePlace") + assert result == DescriptionResolution("Hello from AgilePlace", True, False, False, None) + _assert_invariant(None, result) + + +def test_seeding_both_nonempty_and_different_is_a_conflict_that_writes_nothing(): + result = resolve_description(None, None, "GH text", "AP text") + assert result.write_gh is False + assert result.write_ap is False + assert result.conflict is True + assert result.merged == "" + assert result.warning is not None + _assert_invariant(None, result) + + +def test_steady_state_neither_side_changed_writes_nothing(): + result = resolve_description("Same text", "Same text", "Same text", "Same text") + assert result == DescriptionResolution("Same text", False, False, False, None) + _assert_invariant("Same text", result) + + +def test_truncated_steady_state_compares_ap_side_against_its_own_written_form(): + # desc_base is the FULL agreed canonical; desc_ap_written is the canonical of the truncated + # text a prior run actually wrote. Neither side changed since ITS OWN reference point, so this + # must be silent steady state -- comparing ap_canonical against the full base instead would + # wrongly look like an AP-side edit on every subsequent run. + full = "A very long description " * 50 + truncated = full[:100] + "...[truncated by sync]" + result = resolve_description(full, truncated, full, truncated) + assert result == DescriptionResolution(full, False, False, False, None) + _assert_invariant(full, result) + + +def test_ap_edit_on_top_of_a_truncated_stub_is_not_blindly_pushed_to_github(): + # desc_base is the FULL agreed canonical; desc_ap_written is only the canonical of the + # TRUNCATED stand-in a prior run actually wrote (see the truncated-steady-state test above). + # If the card's truncated stub is edited further (a comment appended, a typo fixed on the + # visible portion, ...), ap_canonical now differs from desc_ap_written -- but ap_canonical is + # still only that truncated stub, never the untruncated tail that lives solely in `base`. + # Blindly promoting it to `merged` and pushing it to GitHub would permanently destroy the lost + # tail with no warning (critical review finding for issue #65). This must degrade to the same + # warn-and-skip conflict policy as a genuine both-sides conflict: nothing is written, the full + # base stays put, and a human is told to reconcile by hand. + full = "A very long description " * 50 + truncated = full[:100] + "...[truncated by sync]" + edited_truncated = truncated + " one more sentence appended on the card" + result = resolve_description(full, truncated, full, edited_truncated) + assert result.write_gh is False + assert result.write_ap is False + assert result.conflict is True + assert result.merged == full + assert result.warning is not None + _assert_invariant(full, result) + + +def test_gh_only_changed_propagates_to_agileplace(): + result = resolve_description("Old text", "Old text", "New GH text", "Old text") + assert result == DescriptionResolution("New GH text", False, True, False, None) + _assert_invariant("Old text", result) + + +def test_ap_only_changed_propagates_to_github(): + result = resolve_description("Old text", "Old text", "Old text", "New AP text") + assert result == DescriptionResolution("New AP text", True, False, False, None) + _assert_invariant("Old text", result) + + +def test_both_changed_to_different_values_is_a_conflict_that_writes_nothing(): + result = resolve_description("Old text", "Old text", "GH new", "AP new") + assert result.write_gh is False + assert result.write_ap is False + assert result.conflict is True + assert result.merged == "Old text" + assert result.warning is not None + _assert_invariant("Old text", result) + + +def test_both_changed_but_independently_converged_on_the_same_value_is_not_a_conflict(): + result = resolve_description("Old text", "Old text", "Same new text", "Same new text") + assert result == DescriptionResolution("Same new text", False, False, False, None) + _assert_invariant("Old text", result) + + +# ===================================================================================== +# None and "" are indistinguishable inputs on both reference points +# ===================================================================================== + +def test_none_and_empty_string_base_normalize_identically(): + assert resolve_description(None, None, "x", "") == resolve_description("", "", "x", "") + + +def test_none_and_empty_string_ap_written_base_normalize_identically(): + assert (resolve_description("b", None, "b", "y") + == resolve_description("b", "", "b", "y")) + + +# ===================================================================================== +# _canonicalize_gh_body -- genuine md->html->md round trip, self-composition-idempotent +# ===================================================================================== + +def test_canonicalize_gh_body_normalizes_none_and_missing_to_empty_string(): + assert _canonicalize_gh_body(None) == "" + assert _canonicalize_gh_body("") == "" + + +def test_canonicalize_gh_body_is_self_composition_idempotent(): + body = "Some **bold** and *italic* text\n\n- item one\n- item two\n" + once = _canonicalize_gh_body(body) + twice = _canonicalize_gh_body(once) + assert once == twice + + +# ===================================================================================== +# _canonicalize_ap_description -- one-directional html->md, NOT self-composition-idempotent; +# the real invariant is a round trip THROUGH HTML (spike finding #3 correction). +# ===================================================================================== + +def test_canonicalize_ap_description_normalizes_none_and_missing_to_empty_string(): + assert _canonicalize_ap_description(None) == "" + assert _canonicalize_ap_description("") == "" + + +def test_canonicalize_ap_description_round_trips_through_html(): + html = "

Intro bold and italic text with code().

" + once = _canonicalize_ap_description(html) + rerendered_html = richtext.markdown_to_leankit_html(once) + twice = _canonicalize_ap_description(rerendered_html) + assert once == twice + + +def test_canonicalize_ap_description_is_not_self_composition_idempotent(): + # Feeding the function's own Markdown output back in AS IF it were HTML is a type mismatch, + # not a no-op: the HTML->Markdown walker escapes the literal '*' characters it sees as plain + # text, so a naive f(f(x)) != f(x) here -- this is the CORRECTED invariant's whole point. + html = "

Intro bold text.

" + once = _canonicalize_ap_description(html) + naive_twice = _canonicalize_ap_description(once) + assert once != naive_twice + + +# ===================================================================================== +# _truncate_for_agileplace -- binary-search truncation (spike finding #1: replaces an O(n^2) +# per-word shrink loop that took 52+s on a 25k-char body). Pins: under-limit passthrough, +# over-limit truncation that fits max_length with TRUNCATION_MARKER appended, graceful +# degeneration on a tiny max_length, and an O(log n)-renders wall-clock bound on a >=20k body. +# ===================================================================================== + +def test_truncate_under_limit_returns_full_html_unchanged(): + markdown = "A short description that is well under any reasonable limit." + full_html = richtext.markdown_to_leankit_html(markdown) + html, was_truncated = _truncate_for_agileplace(markdown, max_length=len(full_html) + 100) + assert html == full_html + assert was_truncated is False + + +def test_truncate_exactly_at_limit_returns_full_html_unchanged(): + markdown = "Exactly at the boundary." + full_html = richtext.markdown_to_leankit_html(markdown) + html, was_truncated = _truncate_for_agileplace(markdown, max_length=len(full_html)) + assert html == full_html + assert was_truncated is False + + +def test_truncate_over_limit_fits_max_length_and_appends_marker(): + markdown = " ".join(f"word{i}" for i in range(2000)) + max_length = 500 + html, was_truncated = _truncate_for_agileplace(markdown, max_length=max_length) + assert was_truncated is True + assert len(html) <= max_length + assert html.endswith(TRUNCATION_MARKER) + + +def test_truncate_never_negative_length_slices_on_tiny_input(): + # A single "word" with no whitespace at all -- snapping to a whitespace boundary must degrade + # to an empty prefix rather than slicing with a negative index. + markdown = "x" * 50 + html, was_truncated = _truncate_for_agileplace(markdown, max_length=10) + assert was_truncated is True + assert html.endswith(TRUNCATION_MARKER) + + +def test_truncate_degenerate_tiny_max_length_degrades_to_marker_only(): + markdown = "Several words that would normally survive truncation easily here." + html, was_truncated = _truncate_for_agileplace(markdown, max_length=1) + assert was_truncated is True + # Even a budget smaller than the marker itself must terminate and produce the marker-only + # result, never raise and never loop forever. + assert html == TRUNCATION_MARKER + + +def test_truncate_result_never_exceeds_max_length_when_achievable(): + markdown = "Alpha beta gamma delta epsilon zeta eta theta iota kappa. " * 20 + max_length = 200 + html, was_truncated = _truncate_for_agileplace(markdown, max_length=max_length) + assert was_truncated is True + assert len(html) <= max_length + + +def test_truncate_large_body_uses_a_logarithmic_number_of_renders_not_linear(): + # Pins the O(log n)-RENDERS invariant _truncate_for_agileplace's own docstring claims, by + # counting actual calls to richtext.markdown_to_leankit_html instead of inferring it from + # wall-clock time. A wall-clock ceiling is only a proxy: a regression to a coarser-but-still- + # polynomial scheme (fixed-percentage chunks, an accidental O(sqrt(n)) loop, ...) could still + # finish under a generous time budget and pass, and a genuinely correct O(log n) run could + # exceed a tight one on slow/loaded CI hardware. Counting calls pins the claim directly. + # Wraps (never replaces) the real renderer so truncation behavior itself is unaffected. + markdown = ("The quick brown fox jumps over the lazy dog. " * 1500) + assert len(markdown) >= 20_000 + real_render = richtext.markdown_to_leankit_html + calls = [] + + def _counting_render(md): + calls.append(md) + return real_render(md) + + with patch("description_sync.richtext.markdown_to_leankit_html", side_effect=_counting_render): + html, was_truncated = _truncate_for_agileplace(markdown, max_length=5000) + + assert was_truncated is True + assert len(html) <= 5000 + # A binary search over len(markdown) candidate cuts takes ~log2(n) iterations plus a couple of + # fixed renders (the initial full-length probe, the final confirmed cut) -- nowhere near + # proportional to len(markdown). A regression back to a per-word/per-chunk shrink loop would + # blow this bound by orders of magnitude on a ~70,000-char input. + max_expected_renders = math.ceil(math.log2(len(markdown))) + 4 + assert len(calls) <= max_expected_renders, ( + f"_truncate_for_agileplace made {len(calls)} renders (expected <= {max_expected_renders}) " + "-- the O(log n) fix may have regressed to a linear/near-linear scheme" + ) + + +# ===================================================================================== +# sync_description -- wiring entry point + coupled base-advance gate (issue #65 task 4). +# +# Mirrors sync_dates' merge-base contract (test_sync_dates.py) exactly: desc_base and +# desc_ap_written only ever advance TOGETHER, and only when apply is True AND the GitHub-side +# write is confirmed (gh_write_ok -- trivially True whenever no GitHub write was needed at all). +# The AgilePlace-side queue write is unconditional -- it fires whenever write_ap is True +# regardless of apply/gh_write_ok, and that firing must never leak into the base-advance gate. +# ===================================================================================== + +def test_sync_description_dry_run_does_not_advance_base_despite_queued_ap_write(): + # gh-changed-only (seeding): write_ap fires and queue is called unconditionally even though + # apply=False -- but the merge base must NOT advance on a dry run. + issue = _issue(body="Hello from GitHub") + card = _card() + state = {ISSUE_URL: {}} + queue = _Queue() + with patch("description_sync.agileplace_description.card_description", return_value=""), \ + patch("description_sync.agileplace_description.op_description", return_value="OP") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body") as edit_mock: + sync_description(_cfg(), False, issue, card, state, queue) + op_mock.assert_called_once() + edit_mock.assert_not_called() + assert len(queue.calls) == 1 + assert "desc_base" not in state[ISSUE_URL] + assert "desc_ap_written" not in state[ISSUE_URL] + + +def test_sync_description_failed_gh_write_blocks_base_advance(): + # ap-changed-only (seeding): write_gh fires but the GitHub write is reported as failed/skipped + # -- neither field may advance, even though apply is True. + issue = _issue(body="") + card = _card() + state = {ISSUE_URL: {}} + queue = _Queue() + with patch("description_sync.agileplace_description.card_description", return_value="Hello from AgilePlace"), \ + patch("description_sync.agileplace_description.op_description") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body", return_value=False) as edit_mock: + sync_description(_cfg(), True, issue, card, state, queue) + edit_mock.assert_called_once_with(_cfg(), True, issue["number"], "Hello from AgilePlace") + op_mock.assert_not_called() + assert queue.calls == [] + assert "desc_base" not in state[ISSUE_URL] + assert "desc_ap_written" not in state[ISSUE_URL] + + +def test_sync_description_conflict_makes_no_writes_and_no_advance(): + issue = _issue(body="GH text") + card = _card() + state = {ISSUE_URL: {}} + queue = _Queue() + with patch("description_sync.agileplace_description.card_description", return_value="AP text"), \ + patch("description_sync.agileplace_description.op_description") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body") as edit_mock: + sync_description(_cfg(), True, issue, card, state, queue) + edit_mock.assert_not_called() + op_mock.assert_not_called() + assert queue.calls == [] + assert "desc_base" not in state[ISSUE_URL] + assert "desc_ap_written" not in state[ISSUE_URL] + + +def test_sync_description_confirmed_gh_write_advances_both_fields_together(): + # ap-changed-only, confirmed GitHub write -> both fields advance to the SAME merged value + # (write_ap never fired here, so desc_ap_written simply reflects the unchanged AP canonical). + issue = _issue(body="") + card = _card() + state = {ISSUE_URL: {}} + queue = _Queue() + with patch("description_sync.agileplace_description.card_description", return_value="Hello from AgilePlace"), \ + patch("description_sync.agileplace_description.op_description") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body", return_value=True) as edit_mock: + sync_description(_cfg(), True, issue, card, state, queue) + edit_mock.assert_called_once_with(_cfg(), True, issue["number"], "Hello from AgilePlace") + op_mock.assert_not_called() + assert queue.calls == [] + assert state[ISSUE_URL]["desc_base"] == "Hello from AgilePlace" + assert state[ISSUE_URL]["desc_ap_written"] == "Hello from AgilePlace" + + +def test_sync_description_confirmed_ap_write_advances_both_fields_together(): + # gh-changed-only (seeding), no GitHub write needed at all -> gh_write_ok is trivially True, + # so both fields advance even though the only write that happened was the queued AP op. + issue = _issue(body="Hello from GitHub") + card = _card() + state = {ISSUE_URL: {}} + queue = _Queue() + written_html = richtext.markdown_to_leankit_html("Hello from GitHub") + expected_ap_written = _canonicalize_ap_description(written_html) + with patch("description_sync.agileplace_description.card_description", return_value=""), \ + patch("description_sync.agileplace_description.op_description", return_value="OP") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body") as edit_mock: + sync_description(_cfg(), True, issue, card, state, queue) + edit_mock.assert_not_called() + op_mock.assert_called_once_with(written_html) + assert queue.calls == [(card, ["OP"], "description")] + assert state[ISSUE_URL]["desc_base"] == "Hello from GitHub" + assert state[ISSUE_URL]["desc_ap_written"] == expected_ap_written + + +def test_sync_description_never_reads_a_plan_only_dry_run_card(): + # Discovered running the wiring end-to-end (tests/test_run.py's dry-run path, issue #65 task + # 5/7): a dry-run-only card carries agileplace.create_card's synthetic "_planOnly" marker and + # has no server-side identity yet, so it never has a 'description' key. Without a guard, + # agileplace_description.card_description()'s lazy get_card() fallback fires a live GET for an id that was + # never created on the server -- exactly the network read sync.py's own dependency-sync loop + # already refuses for plan-only cards ("a fresh card has no server-side dependencies; never + # read a plan-only id"). sync_description must apply that same convention: treat a plan-only + # card's AgilePlace-side description as "" without calling agileplace_description.card_description() at all. + issue = _issue(body="Hello from GitHub") + card = {**_card(), "_planOnly": True} + state = {ISSUE_URL: {}} + queue = _Queue() + written_html = richtext.markdown_to_leankit_html("Hello from GitHub") + with patch("description_sync.agileplace_description.card_description") as card_description_mock, \ + patch("description_sync.agileplace_description.op_description", return_value="OP") as op_mock, \ + patch("description_sync.ghkit.edit_issue_body") as edit_mock: + sync_description(_cfg(), False, issue, card, state, queue) + card_description_mock.assert_not_called() + edit_mock.assert_not_called() + op_mock.assert_called_once_with(written_html) + assert queue.calls == [(card, ["OP"], "description")] diff --git a/tests/test_description_sync_wiring_fixtures.py b/tests/test_description_sync_wiring_fixtures.py new file mode 100644 index 0000000..388db19 --- /dev/null +++ b/tests/test_description_sync_wiring_fixtures.py @@ -0,0 +1,68 @@ +"""Guards issue #65 task 5's fixture-repair invariant: wiring sync_description into sync.py's +per-issue loop must never let a pre-existing card fixture in any file exercising sync.main() +fall through to a real agileplace.api() call. + +agileplace_description.card_description() takes a zero-I/O path whenever the card dict already carries a +'description' key (even ""); a fixture that omits it makes sync_description's +card_description(cfg, card) call fall back to agileplace.get_card(), which none of these +files mock -- so it hits the real HTTP client. Confirmed live in the design spike: a real host +answers with HTTP 401 -> SystemExit inside files that patch agileplace at the function level +(not the transport), and test_run.py's own request-dispatch stub (FixtureWorld.open_url) raises +its own AssertionError("unexpected AgilePlace request") for the same reason -- neither is a +legitimate assertion failure, both are this exact regression. + +Rather than re-deriving by static analysis which fixtures reach the per-issue loop (fragile +across the differently-shaped harnesses in play -- test_sync_main.py's shared _mock_io, +test_vetting_latch.py's reuse of it, test_run.py's own FixtureWorld HTTP stub, and the +per-file card builders in the hierarchy/retired/coherence/contested-card suites), this runs +each file as its own subprocess and requires zero failures/errors: the same boundary the +spike's regression was actually caught at. + +Run: pytest -q tests/test_description_sync_wiring_fixtures.py +""" +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +# Every file whose card fixtures reach sync.main()'s per-issue loop -- and therefore +# sync_description's agileplace_description.card_description() call -- belongs here. This +# includes task 5/7's original three plus the hierarchy/retired/coherence/contested-card +# suites, all of which needed the same 'description' key fixture repair. +WIRED_TEST_FILES = ( + "tests/test_sync_main.py", + "tests/test_sync_intake_call_site.py", + "tests/test_vetting_latch.py", + "tests/test_run.py", + "tests/test_hierarchy_ownership.py", + "tests/test_retired_issues.py", + "tests/test_sync_card_coherence.py", + "tests/test_sync_contested_cards.py", +) + + +def test_no_wired_test_file_lets_a_card_fixture_reach_real_agileplace_api(): + failures = [] + for relative_path in WIRED_TEST_FILES: + result = subprocess.run( + [sys.executable, "-m", "pytest", "-q", relative_path], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=120, + ) + if result.returncode != 0: + summary = result.stdout.strip().splitlines()[-1] if result.stdout.strip() else "" + failures.append( + f"{relative_path}: exit={result.returncode} summary={summary!r}\n" + f"{result.stdout[-3000:]}" + ) + + assert not failures, ( + "one or more wired test files let a card fixture reach sync_description's " + "agileplace_description.card_description() fallback (real agileplace.api() call) -- give the fixture " + "a 'description' key or explicitly mock agileplace.get_card:\n\n" + "\n\n".join(failures) + ) diff --git a/tests/test_ghkit_edit_issue_body.py b/tests/test_ghkit_edit_issue_body.py new file mode 100644 index 0000000..2148c67 --- /dev/null +++ b/tests/test_ghkit_edit_issue_body.py @@ -0,0 +1,186 @@ +"""Unit tests for issue #65 Task 1/7: ghkit.edit_issue_body() (the GitHub-side client-boundary +write for description sync) and list_issues()'s null-safe 'body' field addition. + +edit_issue_body follows the exact dry-run/apply/boundary-validation idiom already established by +create_issue/edit_label: apply=False makes zero calls to run() and returns False; apply=True pipes +the body through run()'s stdin `input=` passthrough (never argv interpolation) and returns True on +success; a malformed `number`/`body` raises ValueError before any I/O. + +Run: pytest -q tests/test_ghkit_edit_issue_body.py +""" +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path +from unittest.mock import Mock + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import ghkit # noqa: E402 + + +# --- dry-run: zero calls to the transport boundary, returns False ------------ + +def test_edit_issue_body_dry_run_makes_zero_run_calls_and_returns_false(monkeypatch, capsys): + calls = [] + monkeypatch.setattr(ghkit, "run", lambda *a, **k: calls.append((a, k))) + + result = ghkit.edit_issue_body({}, False, 42, "new body") + + assert result is False + assert calls == [] + assert capsys.readouterr().out.startswith("DRY") + + +def test_edit_issue_body_dry_run_prints_issue_number(monkeypatch, capsys): + monkeypatch.setattr(ghkit, "run", + lambda *a, **k: (_ for _ in ()).throw(AssertionError("must not call run"))) + + ghkit.edit_issue_body({}, False, 42, "body") + + assert "42" in capsys.readouterr().out + + +# --- apply=True: exact argv shape + stdin body passthrough, returns True ----- + +def test_edit_issue_body_apply_calls_run_with_body_file_stdin_and_input_kwarg(monkeypatch): + captured = {} + + def fake_run(cfg, args, **kwargs): + captured["args"] = args + captured["kwargs"] = kwargs + return Mock(stdout="") + + monkeypatch.setattr(ghkit, "run", fake_run) + + result = ghkit.edit_issue_body({}, True, 42, "the new body") + + args = captured["args"] + assert args[:2] == ["issue", "edit"] + assert args[2] == "42" + assert "--body-file" in args and args[args.index("--body-file") + 1] == "-" + assert captured["kwargs"].get("input") == "the new body" + assert result is True + + +def test_edit_issue_body_calls_run_exactly_once(monkeypatch): + calls = [] + + def fake_run(cfg, args, **kwargs): + calls.append((args, kwargs)) + return Mock(stdout="") + + monkeypatch.setattr(ghkit, "run", fake_run) + + ghkit.edit_issue_body({}, True, 42, "body") + + assert len(calls) == 1 + + +def test_edit_issue_body_accepts_empty_string_body(monkeypatch): + """Clearing a description entirely is a legitimate write -- an empty-string body must not be + rejected by the boundary validation.""" + captured = {} + monkeypatch.setattr(ghkit, "run", + lambda cfg, args, **k: (captured.update(k), Mock(stdout=""))[1]) + + result = ghkit.edit_issue_body({}, True, 1, "") + + assert result is True + assert captured.get("input") == "" + + +# --- apply=True: transport failures propagate uncaught ----------------------- + +@pytest.mark.parametrize("exc", [ + subprocess.CalledProcessError(returncode=1, cmd=["gh"]), + subprocess.TimeoutExpired(cmd=["gh"], timeout=60), +]) +def test_edit_issue_body_propagates_run_failures_uncaught(monkeypatch, exc): + def fake_run(cfg, args, **k): + raise exc + + monkeypatch.setattr(ghkit, "run", fake_run) + + with pytest.raises(type(exc)): + ghkit.edit_issue_body({}, True, 42, "body") + + +# --- number/body validated at the boundary, before any I/O ------------------- + +@pytest.mark.parametrize("number", [None, "42", 0, -1, 4.2, True]) +@pytest.mark.parametrize("apply", [True, False]) +def test_edit_issue_body_rejects_an_unusable_number_before_reaching_run(monkeypatch, number, apply): + monkeypatch.setattr(ghkit, "run", lambda *a, **k: (_ for _ in ()).throw( + AssertionError("must not call run for an unusable number"))) + + with pytest.raises(ValueError, match="number"): + ghkit.edit_issue_body({}, apply, number, "body") + + +@pytest.mark.parametrize("body", [None, 42, ["not", "a", "string"]]) +@pytest.mark.parametrize("apply", [True, False]) +def test_edit_issue_body_rejects_a_non_string_body_before_reaching_run(monkeypatch, body, apply): + monkeypatch.setattr(ghkit, "run", lambda *a, **k: (_ for _ in ()).throw( + AssertionError("must not call run for a non-string body"))) + + with pytest.raises(ValueError, match="body"): + ghkit.edit_issue_body({}, apply, 1, body) + + +# --- list_issues(): null-safe 'body' field addition (issue #65 struct #5) ---- + +def _raw_issue(**overrides) -> dict: + base = { + "number": 1, "title": "T", "state": "OPEN", "stateReason": "", + "labels": [], "milestone": None, "assignees": [], + "url": "https://github.com/o/r/issues/1", + } + base.update(overrides) + return base + + +def test_list_issues_includes_body_when_present(monkeypatch): + monkeypatch.setattr(ghkit, "run", + lambda *a, **k: Mock(stdout=json.dumps([_raw_issue(body="the body text")]))) + + issues = ghkit.list_issues({}) + + assert issues[0]["body"] == "the body text" + + +def test_list_issues_normalizes_missing_body_to_empty_string(monkeypatch): + monkeypatch.setattr(ghkit, "run", lambda *a, **k: Mock(stdout=json.dumps([_raw_issue()]))) + + issues = ghkit.list_issues({}) + + assert issues[0]["body"] == "" + + +def test_list_issues_normalizes_null_body_to_empty_string(monkeypatch): + monkeypatch.setattr(ghkit, "run", + lambda *a, **k: Mock(stdout=json.dumps([_raw_issue(body=None)]))) + + issues = ghkit.list_issues({}) + + assert issues[0]["body"] == "" + + +def test_list_issues_requests_body_field_in_gh_json_flag(monkeypatch): + captured = {} + + def fake_run(cfg, args, **kwargs): + captured["args"] = args + return Mock(stdout="[]") + + monkeypatch.setattr(ghkit, "run", fake_run) + + ghkit.list_issues({}) + + json_flag_index = captured["args"].index("--json") + fields = captured["args"][json_flag_index + 1].split(",") + assert "body" in fields diff --git a/tests/test_hierarchy_ownership.py b/tests/test_hierarchy_ownership.py index 035c749..5656f89 100644 --- a/tests/test_hierarchy_ownership.py +++ b/tests/test_hierarchy_ownership.py @@ -39,6 +39,8 @@ def _card(number: int, custom_id: str, *, url: str | None = None, "laneId": None, "blockedStatus": {"isBlocked": False, "reason": ""}, "childCards": [{"id": child_id} for child_id in children], + # issue #65: keeps agileplace_description.card_description() on its zero-I/O path. + "description": "", } return {**card, **({"externalLink": {"url": url}} if url else {})} @@ -52,6 +54,7 @@ def _config(tmp_path: Path) -> dict: "label_sync_ignore": frozenset(), "stage_lane_map": {}, "gh_project": {}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } diff --git a/tests/test_retired_issues.py b/tests/test_retired_issues.py index c1b49fc..4fae184 100644 --- a/tests/test_retired_issues.py +++ b/tests/test_retired_issues.py @@ -39,6 +39,7 @@ def _config(tmp_path) -> dict: "label_sync_ignore": frozenset(), "stage_lane_map": {}, "gh_project": {}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } @@ -86,6 +87,8 @@ def _card(number: int, lane_id: str, *, blocked: bool) -> dict: "blockedStatus": {"isBlocked": blocked, "reason": "Blocked by #10" if blocked else ""}, "plannedStart": None, "plannedFinish": None, + # issue #65: keeps agileplace_description.card_description() on its zero-I/O path. + "description": "", } diff --git a/tests/test_run.py b/tests/test_run.py index 98dd50d..f2af37b 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -119,6 +119,10 @@ def __init__(self): "plannedFinish": None, "blockedStatus": {"isBlocked": False, "reason": ""}, "childCards": [], + # issue #65: keeps agileplace_description.card_description() on its zero-I/O path -- without this + # key it falls back to the real (unmocked) GET card/{id}, which this fixture's own + # open_url dispatch doesn't expect and raises "unexpected AgilePlace request". + "description": "", } def run_process(self, argv, **_kwargs): @@ -200,6 +204,7 @@ def open_url(self, request, **_kwargs): "plannedStart": None, "plannedFinish": None, "blockedStatus": {"isBlocked": False, "reason": ""}, + "description": "", # issue #65: same zero-I/O contract as epic_card above }, }) if method in {"POST", "DELETE"} and path == "card/connections": @@ -411,6 +416,7 @@ def __init__(self): "plannedFinish": None, "blockedStatus": {"isBlocked": False, "reason": ""}, "childCards": [], + "description": "", # issue #65: same zero-I/O contract as FixtureWorld.epic_card } def run_process(self, argv, **kwargs): diff --git a/tests/test_smoke.py b/tests/test_smoke.py index 2b9d856..70f9890 100644 --- a/tests/test_smoke.py +++ b/tests/test_smoke.py @@ -54,7 +54,7 @@ def __init__(self, *, accept_stale: bool = False, fail_child_create_body: str | self.writes: list[tuple[str, str]] = [] self.cards: dict[str, dict] = { "P1": {"id": "P1", "title": "Existing card", "customId": "EX-1", - "laneId": "L1", "tags": [], "version": 3}, + "laneId": "L1", "tags": [], "version": 3, "description": ""}, } self.children: dict[str, list[str]] = {} self.dependencies: dict[str, list[dict]] = {} @@ -114,7 +114,7 @@ def _create(self, url: str, body: dict): self.created_custom_ids.append(body["customId"]) card = {"id": card_id, "title": body["title"], "customId": body["customId"], "laneId": body.get("laneId"), "tags": [], "version": 1, - "plannedStart": None, "plannedFinish": None, + "plannedStart": None, "plannedFinish": None, "description": "", "blockedStatus": {"isBlocked": False, "reason": ""}} if "externalLink" in body: card["externalLink"] = body["externalLink"] @@ -151,6 +151,8 @@ def _patch(self, req, card_id: str, ops: list): elif op["path"] == "/externalLink": if not self.ignore_external_link: card["externalLink"] = op["value"] + elif op["path"] == "/description": + card["description"] = op["value"] elif op["path"] == "/isBlocked": card["blockedStatus"]["isBlocked"] = op["value"] elif op["path"] == "/blockReason": @@ -249,6 +251,8 @@ def test_confirmed_run_executes_whole_sequence_and_cleans_up(tenant_env, capsys) ("POST", "card/dependency"), # dependency create (child dependsOn parent) ("POST", "card/dependency"), # duplicate-create fact-finding probe ("DELETE", "card/dependency"), # dependency delete + ("PATCH", "card/S1"), # description write round-trip + ("PATCH", "card/S1"), # description length probe ("PATCH", "card/S1"), # deliberate stale-version probe ("DELETE", "card/S2"), # cleanup child ("DELETE", "card/S1"), # cleanup parent @@ -263,6 +267,8 @@ def test_confirmed_run_executes_whole_sequence_and_cleans_up(tenant_env, capsys) assert "planned" in out # planned-date round-trip reported assert "duplicate create rejected" in out # the live 409 contract, mirrored by the double assert "Dependency already exists" in out + assert "description write round-trip" in out + assert "description length probe" in out def test_unexpected_duplicate_create_failure_fails_the_run(tenant_env, capsys): diff --git a/tests/test_sync_card_coherence.py b/tests/test_sync_card_coherence.py index 0cacb26..22b7e10 100644 --- a/tests/test_sync_card_coherence.py +++ b/tests/test_sync_card_coherence.py @@ -84,6 +84,8 @@ def _card(card_id: str, custom_id: str, urls: list[str], *, lane_id: str = "L-EL "tags": [], "plannedStart": None, "plannedFinish": None, + # issue #65: keeps agileplace_description.card_description() on its zero-I/O path. + "description": "", } @@ -96,6 +98,7 @@ def _cfg(tmp_path) -> dict: "label_sync_ignore": frozenset(), "stage_lane_map": {}, "gh_project": {}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } diff --git a/tests/test_sync_contested_cards.py b/tests/test_sync_contested_cards.py index 46e15e9..bf60e9a 100644 --- a/tests/test_sync_contested_cards.py +++ b/tests/test_sync_contested_cards.py @@ -45,6 +45,7 @@ def _config(tmp_path) -> dict: "label_sync_ignore": frozenset(), "stage_lane_map": {}, "gh_project": {}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } @@ -60,6 +61,8 @@ def _card_with_urls(card_id: str, custom_id: str, urls: list[str], *, lane_id: s "tags": [], "plannedStart": None, "plannedFinish": None, + # issue #65: keeps agileplace_description.card_description() on its zero-I/O path. + "description": "", } diff --git a/tests/test_sync_description_call_site.py b/tests/test_sync_description_call_site.py new file mode 100644 index 0000000..ed14649 --- /dev/null +++ b/tests/test_sync_description_call_site.py @@ -0,0 +1,85 @@ +"""sync.main()'s description-sync call site (Task 6/7, issue #65). + +Thin, isolated from the large test_sync_main.py suite: this file's only concern is that main()'s +per-issue loop actually calls description_sync.sync_description() with the right positional +arguments (cfg, apply, issue, card, issues_state, queue) -- once per syncable issue, after the +lane/metadata/dates steps for that issue. It does not re-verify sync_description()'s own merge/ +truncation/conflict behavior -- that is tests/test_description_sync.py's job -- so +sync_description itself is monkeypatched here, matching the low-level-transport-boundary +convention this repo's other main()-level call-site tests use (test_sync_intake_call_site.py +patches intake.promote() the same way; patching the collaborator, not its internals). + +sync.py imports the name directly (`from description_sync import sync_description`), so the call +site lives in sync's own namespace -- the patch target is "sync.sync_description", not +"description_sync.sync_description". + +Run: pytest -q tests/test_sync_description_call_site.py +""" +from __future__ import annotations + +import sys +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import sync # noqa: E402 + +# _mock_io/_card/_cfg are test_sync_main.py's richer I/O-boundary-mocking helpers -- reused here +# rather than duplicated, matching test_sync_intake_call_site.py's own precedent. +from test_sync_main import _card, _cfg, _issue, _mock_io # noqa: E402 + +ISSUE_URL = "https://github.com/acme/repo/issues/1" + + +def test_main_calls_sync_description_once_per_issue_with_expected_args(tmp_path): + state_file = tmp_path / ".sync-state.json" + cfg = _cfg(tmp_path) + issue = _issue() + card = _card() + stack, _run_mock, _patch_card_mock, _create_card_mock = _mock_io( + card, ({}, []), field_meta_return=None) + sync_description_mock = stack.enter_context(patch("sync.sync_description")) + + with stack, patch("sync.env_config", return_value=cfg), patch("sync.STATE_FILE", state_file), \ + patch("sys.argv", ["sync.py", "--apply"]): + sync.main() + + sync_description_mock.assert_called_once() + call_cfg, call_apply, call_issue, call_card, call_issues_state, call_queue = ( + sync_description_mock.call_args.args) + assert call_cfg is cfg + assert call_apply is True + # main() annotates has_open_pr onto each issue (issue #14, pre-#65) before the per-issue loop, + # so the issue sync_description receives carries that field too -- same precedent + # test_sync_intake_call_site.py's own comment documents for intake.promote()'s issues arg. + assert call_issue == {**issue, "has_open_pr": False} + assert call_card["id"] == card["id"] + # sync_metadata (labels/milestone) runs on this same issue immediately before sync_description + # in the per-issue loop, so by the time sync_description is called issues_state already carries + # the keys sync_metadata itself learned this run -- this call site doesn't own those keys, it + # only asserts sync_description sees the same live per-issue state dict main() threads through. + assert call_issues_state == { + ISSUE_URL: {"card_id": "C1", "labels": [], "milestone": None}, + } + assert callable(call_queue) + + +def test_main_never_reaches_real_sync_description_for_an_unresolved_card(tmp_path): + """A syncable issue with no matching/created card this run (card_for(issue) is falsy) must + `continue` before reaching sync_description -- mirrors the existing lane/metadata/dates guard + a few lines above the call site.""" + state_file = tmp_path / ".sync-state.json" + cfg = _cfg(tmp_path) + stack, _run_mock, _patch_card_mock, _create_card_mock = _mock_io( + _card(), ({}, []), field_meta_return=None, existing_cards=[]) + sync_description_mock = stack.enter_context(patch("sync.sync_description")) + # No card matches this issue and creation is suppressed by patching create_card to return {} + # (no "id"), so card_for(issue) stays falsy and the per-issue loop's `continue` guard fires + # before ever reaching sync_description. + + with stack, patch("sync.env_config", return_value=cfg), patch("sync.STATE_FILE", state_file), \ + patch("sys.argv", ["sync.py", "--apply"]): + sync.main() + + sync_description_mock.assert_not_called() diff --git a/tests/test_sync_intake_call_site.py b/tests/test_sync_intake_call_site.py index 2eb9065..2db8b48 100644 --- a/tests/test_sync_intake_call_site.py +++ b/tests/test_sync_intake_call_site.py @@ -39,9 +39,11 @@ def _issue(): def _card(): + # "description": "" (issue #65) keeps agileplace_description.card_description() on its zero-I/O path. return {"id": "C1", "version": 1, "customId": "1", "externalLink": {"url": ISSUE_URL}, "tags": [], - "plannedStart": None, "plannedFinish": None, "laneId": "LANE1"} + "plannedStart": None, "plannedFinish": None, "laneId": "LANE1", + "description": ""} def _cfg(tmp_path): @@ -52,6 +54,7 @@ def _cfg(tmp_path): "stage_lane_map": {"Intake": ["New Requests"]}, "gh_project": {"owner": "acme", "number": "7", "status_field": "Status", "start_field": "Start", "target_field": "Target"}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } @@ -110,7 +113,8 @@ def test_main_does_not_create_a_duplicate_card_for_a_resumed_active_issue(tmp_pa active_url = "https://github.com/acme/repo/issues/7" active_issue = {"number": 7, "title": "Raw idea", "state": "OPEN", "labels": [], "milestone": None, "assignees": [], "url": active_url} - intake_card = {"id": "C-intake", "version": 1, "laneId": "lane-intake", "title": "Raw idea"} + intake_card = {"id": "C-intake", "version": 1, "laneId": "lane-intake", "title": "Raw idea", + "description": ""} # issue #65: this card reaches the per-issue loop below intake_lane = {"id": "lane-intake", "title": "New Requests"} cfg = {**_sync_main_cfg(tmp_path), "stage_lane_map": {"Intake": ["New Requests"]}} state_file = tmp_path / ".sync-state.json" diff --git a/tests/test_sync_main.py b/tests/test_sync_main.py index db4e70a..bf33682 100644 --- a/tests/test_sync_main.py +++ b/tests/test_sync_main.py @@ -37,9 +37,13 @@ def _issue(): def _card(): + # "description": "" (issue #65) keeps agileplace_description.card_description() on its zero-I/O path -- + # without the key it falls back to the real (unmocked) agileplace.get_card(), which hits the + # live HTTP client and SystemExits (confirmed live in the design spike). return {"id": "C1", "version": 1, "customId": "1", "externalLink": {"url": ISSUE_URL}, "tags": [], - "plannedStart": "2026-02-01", "plannedFinish": None, "laneId": None} + "plannedStart": "2026-02-01", "plannedFinish": None, "laneId": None, + "description": ""} def _field_meta(): @@ -55,6 +59,7 @@ def _cfg(tmp_path): "stage_lane_map": {}, "gh_project": {"owner": "acme", "number": "7", "status_field": "Status", "start_field": "Start", "target_field": "Target"}, + "ap_description_max_length": 20000, # issue #65: sync_description reads this unconditionally } @@ -210,6 +215,10 @@ def test_legacy_state_resets_merge_base_before_relearning_live_metadata(tmp_path assert state["issues"][ISSUE_URL] == { "card_id": "C1", "labels": ["bug"], "milestone": "1.0", "start": "2026-02-01", "target": None, + # issue #65: sync_description's own additive merge-base keys, advanced together since + # both the issue body and the card description are "" here (no GitHub write needed, so + # gh_write_ok is trivially True) -- unrelated to this test's own labels/dates focus. + "desc_base": "", "desc_ap_written": "", } run_mock.assert_called_once() assert run_mock.call_args.args[1][:2] == ["project", "item-edit"]