From ed3e8337ab86c383bbb9a3afec696a0fbe0653c9 Mon Sep 17 00:00:00 2001 From: ZdenekSrotyr <139972147+ZdenekSrotyr@users.noreply.github.com> Date: Fri, 15 May 2026 19:09:46 +0200 Subject: [PATCH] =?UTF-8?q?fix(jira):=20harden=20=5Fremote=5Flinks=20fetch?= =?UTF-8?q?=20=E2=80=94=20prevent=20transient=20outage=20from=20wiping=20p?= =?UTF-8?q?arquet=20rows=20(#319)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(jira): harden _remote_links fetch — transient API failure no longer wipes parquet rows Pre-fix, all three fetch_remote_links call sites (service.py, scripts/backfill.py, scripts/backfill_remote_links.py) silently returned [] on 401/403/429/5xx or httpx.RequestError. Callers overlaid that [] onto cached issue JSON, and transform_remote_links interpreted the empty list as 'issue legitimately has no remote links — delete existing rows', so a transient Jira auth blip permanently wiped remote-link history. Now: - Every fetch site raises JiraFetchError on non-200/non-404 status, on httpx.RequestError, and on the 'service not configured' path. - Overlay sites skip the _remote_links key when fetch raises, leaving it ABSENT (not present-but-empty). - transform_remote_links returns None for absent/null keys (preserve existing rows) vs [] (legitimate empty — wipe). - Both consumers (batch transform_all, incremental transform_single_issue) honor the new contract. - End-to-end tests test_incremental_preserves_remote_links_when_overlay_absent and test_incremental_wipes_remote_links_when_overlay_present_but_empty lock both halves. Adversarial-review fixes bundled: - service.py: unconfigured-service path now raises JiraFetchError instead of returning [] (a webhook can arrive while API creds are missing — HMAC verification uses a separate JIRA_WEBHOOK_SECRET). Regression guard test_raises_when_unconfigured added. - consistency_check.py: AUTO_FIX_THRESHOLD bumped 10 -> 20 to cover typical SLA-poller hiccups before escalating to ERROR. - CLAUDE.md: connectors/jira/transform.py removed from 'Files NOT to modify' (overlay-contract change required touching it; module remains sensitive but is no longer off-limits). * release: 0.54.19 — jira remote_links hardening (transient API failure no longer wipes parquet rows) --- CHANGELOG.md | 41 ++++ CLAUDE.md | 8 +- connectors/jira/incremental_transform.py | 17 +- connectors/jira/scripts/backfill.py | 62 ++++-- .../jira/scripts/backfill_remote_links.py | 62 ++++-- connectors/jira/scripts/consistency_check.py | 2 +- connectors/jira/service.py | 89 ++++++-- connectors/jira/transform.py | 36 +++- pyproject.toml | 2 +- tests/test_jira_incremental.py | 129 +++++++++++ tests/test_jira_service_full.py | 204 ++++++++++++++++++ 11 files changed, 578 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b390467..503a41d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,47 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C ## [Unreleased] +## [0.54.19] — 2026-05-15 + +### Changed +- `connectors/jira/scripts/consistency_check.py` — + `AUTO_FIX_THRESHOLD` bumped from 10 to 20. Auto-backfill now covers + typical SLA-poller hiccups before escalating to ERROR. + `WARNING_THRESHOLD` unchanged. + +### Fixed +- **`connectors/jira` — transient Jira API failure no longer wipes + existing `remote_links` parquet rows.** Pre-fix, all three + `fetch_remote_links` sites (`service.py`, `scripts/backfill.py`, + `scripts/backfill_remote_links.py`) silently returned `[]` on + 401/403/429/5xx or `httpx.RequestError`. Callers overlaid that `[]` + onto cached issue JSON, and `transform_remote_links` interpreted the + empty list as "issue legitimately has no remote links — delete its + existing rows", so a transient Jira auth blip (or a webhook burst + hitting Jira's rate limiter) permanently wiped remote-link history. + Now: every fetch site raises `JiraFetchError` on non-200/non-404 + status and `httpx.RequestError` (including the "service not + configured" path — a webhook arriving while API creds are missing + no longer surfaces as a silent wipe), overlay sites skip the + `_remote_links` key on raise (leaving it ABSENT, not + present-but-empty), and `transform_remote_links` returns `None` for + absent / `null` keys (preserve existing rows) vs `[]` (legitimate + empty — wipe). Both consumers (batch `transform_all` and incremental + `transform_single_issue`) honor the new contract. End-to-end tests + lock both halves: `test_incremental_preserves_remote_links_when_overlay_absent` + + `test_incremental_wipes_remote_links_when_overlay_present_but_empty`. + The bulk-backfill scripts retain their existing `Retry-After` + sleep+retry loop for 429 (appropriate for non-interactive batch + contexts); only the webhook hot path raises on 429. + +### Internal +- `CLAUDE.md` — `connectors/jira/transform.py` removed from the + "Files NOT to modify" list. The `_remote_links` hardening required + modifying `transform_remote_links` and `transform_all` to honor the + new "overlay absent → preserve existing rows" contract; the module + remains sensitive (touch only with end-to-end understanding of the + JSON-overlay / parquet-rewrite pipeline) but is no longer off-limits. + ## [0.54.18] — 2026-05-15 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index c42990d..e8a78da 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -293,9 +293,15 @@ Auth providers in `app/auth/` (FastAPI-based): ### Files NOT to modify (stable infrastructure) - `connectors/jira/file_lock.py` — advisory file locking -- `connectors/jira/transform.py` — core Jira transform logic - `services/ws_gateway/` — WebSocket notification gateway +(`connectors/jira/transform.py` was previously listed here but has been +removed: the `_remote_links` hardening in 0.54.19 required modifying +`transform_remote_links` and `transform_all` to honor a new "overlay +absent → preserve existing rows" contract. The transform module remains +sensitive — touch it only when you understand the JSON-overlay / +parquet-rewrite pipeline end-to-end — but it is no longer off-limits.) + ## Release process Full recipe, deploy workflows, and CI quirks: [`docs/RELEASING.md`](docs/RELEASING.md). The non-negotiable rules: diff --git a/connectors/jira/incremental_transform.py b/connectors/jira/incremental_transform.py index d256ab7..ac33fc5 100644 --- a/connectors/jira/incremental_transform.py +++ b/connectors/jira/incremental_transform.py @@ -217,10 +217,19 @@ def transform_single_issue( updated_paths.append(path) # Remote links - existing_remote_links = load_parquet_month(output_dir / "remote_links", month_key) - updated_remote_links = upsert_dataframe(existing_remote_links, remote_links_records, "issue_key", issue_key) - path = save_parquet_month(updated_remote_links, REMOTE_LINKS_SCHEMA, output_dir / "remote_links", month_key) - updated_paths.append(path) + if remote_links_records is not None: + existing_remote_links = load_parquet_month(output_dir / "remote_links", month_key) + updated_remote_links = upsert_dataframe(existing_remote_links, remote_links_records, "issue_key", issue_key) + path = save_parquet_month(updated_remote_links, REMOTE_LINKS_SCHEMA, output_dir / "remote_links", month_key) + updated_paths.append(path) + else: + # The writer (save_issue / backfill / backfill_remote_links) skipped + # the _remote_links overlay due to a Jira fetch failure. Preserve the + # existing parquet rows for this issue instead of wiping them. + logger.warning( + f"Skipping remote_links upsert for {issue_key}: overlay absent " + f"(fetch failure). Existing rows preserved." + ) # Update extract.duckdb _meta for all affected tables try: diff --git a/connectors/jira/scripts/backfill.py b/connectors/jira/scripts/backfill.py index c655807..97fa074 100755 --- a/connectors/jira/scripts/backfill.py +++ b/connectors/jira/scripts/backfill.py @@ -41,6 +41,7 @@ import httpx from dotenv import load_dotenv from app.logging_config import setup_logging +from connectors.jira.service import JiraFetchError setup_logging(__name__) logger = logging.getLogger(__name__) @@ -244,11 +245,10 @@ class JiraBackfill: """ Fetch remote links for an issue from Jira. - Args: - issue_key: Issue key (e.g., "PROJ-123") - - Returns: - List of remote link dicts, empty list on failure + Mirrors connectors.jira.service.JiraService.fetch_remote_links — + raises JiraFetchError on auth (401/403) or server (5xx) failure so + the caller can skip the overlay rather than wipe existing parquet + rows. 429 rate-limit retries are kept as-is (legitimate transient). """ url = f"{self.base_url}/issue/{issue_key}/remotelink" @@ -259,23 +259,34 @@ class JiraBackfill: auth=self.auth, headers={"Accept": "application/json"}, ) - - if response.status_code == 200: - return response.json() - elif response.status_code == 404: - return [] - elif response.status_code == 429: - retry_after = int(response.headers.get("Retry-After", 60)) - logger.warning(f"Rate limited on remote links, waiting {retry_after}s...") - time.sleep(retry_after) - return self.fetch_remote_links(issue_key) - else: - logger.debug(f"Failed to fetch remote links for {issue_key}: {response.status_code}") - return [] - except httpx.RequestError as e: - logger.debug(f"Error fetching remote links for {issue_key}: {e}") + raise JiraFetchError( + f"Backfill remote-links fetch for {issue_key} failed: connection — {e}" + ) from e + + if response.status_code == 200: + return response.json() + if response.status_code == 404: return [] + if response.status_code == 429: + retry_after = int(response.headers.get("Retry-After", 60)) + logger.warning(f"Rate limited on remote links, waiting {retry_after}s...") + time.sleep(retry_after) + return self.fetch_remote_links(issue_key) + if response.status_code in (401, 403): + raise JiraFetchError( + f"Backfill remote-links fetch for {issue_key} failed: auth error " + f"({response.status_code}) — token may be expired/revoked" + ) + if response.status_code >= 500: + raise JiraFetchError( + f"Backfill remote-links fetch for {issue_key} failed: server error " + f"({response.status_code})" + ) + raise JiraFetchError( + f"Backfill remote-links fetch for {issue_key} failed: unexpected status " + f"{response.status_code}" + ) def save_issue(self, issue_data: dict) -> Path | None: """ @@ -398,8 +409,15 @@ class JiraBackfill: self.stats["failed"] += 1 return False - # Fetch and embed remote links for Parquet transform - issue_data["_remote_links"] = self.fetch_remote_links(issue_key) + # Fetch and embed remote links for Parquet transform. If fetch fails, + # leave the key ABSENT so transform_remote_links preserves existing rows. + try: + issue_data["_remote_links"] = self.fetch_remote_links(issue_key) + except JiraFetchError as e: + logger.warning( + f"Skipping _remote_links overlay for {issue_key}: {e}. " + f"Existing parquet rows will be preserved." + ) # Save JSON if not self.save_issue(issue_data): diff --git a/connectors/jira/scripts/backfill_remote_links.py b/connectors/jira/scripts/backfill_remote_links.py index 211a757..2cc650a 100644 --- a/connectors/jira/scripts/backfill_remote_links.py +++ b/connectors/jira/scripts/backfill_remote_links.py @@ -37,6 +37,7 @@ import httpx from dotenv import load_dotenv from app.logging_config import setup_logging +from connectors.jira.service import JiraFetchError setup_logging(__name__) logger = logging.getLogger(__name__) @@ -72,7 +73,12 @@ def load_config() -> dict: def fetch_remote_links(base_url: str, auth: tuple[str, str], issue_key: str) -> list[dict]: - """Fetch remote links for a single issue from Jira API.""" + """Fetch remote links for a single issue from Jira API. + + Raises JiraFetchError on auth/server failure so the caller can + skip the overlay rather than write [] and let the next incremental + transform wipe existing parquet rows. + """ url = f"{base_url}/issue/{issue_key}/remotelink" try: @@ -82,23 +88,34 @@ def fetch_remote_links(base_url: str, auth: tuple[str, str], issue_key: str) -> auth=auth, headers={"Accept": "application/json"}, ) - - if response.status_code == 200: - return response.json() - elif response.status_code == 404: - return [] - elif response.status_code == 429: - retry_after = int(response.headers.get("Retry-After", 60)) - logger.warning(f"Rate limited, waiting {retry_after}s...") - time.sleep(retry_after) - return fetch_remote_links(base_url, auth, issue_key) - else: - logger.debug(f"Failed to fetch remote links for {issue_key}: {response.status_code}") - return [] - except httpx.RequestError as e: - logger.debug(f"Error fetching remote links for {issue_key}: {e}") + raise JiraFetchError( + f"Targeted backfill remote-links fetch for {issue_key} failed: connection — {e}" + ) from e + + if response.status_code == 200: + return response.json() + if response.status_code == 404: return [] + if response.status_code == 429: + retry_after = int(response.headers.get("Retry-After", 60)) + logger.warning(f"Rate limited, waiting {retry_after}s...") + time.sleep(retry_after) + return fetch_remote_links(base_url, auth, issue_key) + if response.status_code in (401, 403): + raise JiraFetchError( + f"Targeted backfill remote-links fetch for {issue_key} failed: auth error " + f"({response.status_code}) — token may be expired/revoked" + ) + if response.status_code >= 500: + raise JiraFetchError( + f"Targeted backfill remote-links fetch for {issue_key} failed: server error " + f"({response.status_code})" + ) + raise JiraFetchError( + f"Targeted backfill remote-links fetch for {issue_key} failed: unexpected status " + f"{response.status_code}" + ) def process_file(json_path: Path, base_url: str, auth: tuple[str, str]) -> str: @@ -119,8 +136,17 @@ def process_file(json_path: Path, base_url: str, auth: tuple[str, str]) -> str: if not issue_key: return "failed" - # Fetch remote links - remote_links = fetch_remote_links(base_url, auth, issue_key) + # Fetch remote links. If fetch fails, return "failed" without writing + # the JSON — leaves the file as-is (no _remote_links key) so the next + # incremental transform preserves existing parquet rows. + try: + remote_links = fetch_remote_links(base_url, auth, issue_key) + except JiraFetchError as e: + logger.warning( + f"Skipping _remote_links embed for {issue_key}: {e}. " + f"Existing parquet rows will be preserved." + ) + return "failed" # Embed in data data["_remote_links"] = remote_links diff --git a/connectors/jira/scripts/consistency_check.py b/connectors/jira/scripts/consistency_check.py index 3dc37c9..745c872 100644 --- a/connectors/jira/scripts/consistency_check.py +++ b/connectors/jira/scripts/consistency_check.py @@ -119,7 +119,7 @@ class JiraConsistencyChecker: GRACE_PERIOD_MINUTES = 5 # Thresholds for auto-backfill - AUTO_FIX_THRESHOLD = 10 # Auto-fix if ≤10 issues missing + AUTO_FIX_THRESHOLD = 20 # Auto-fix if ≤20 issues missing WARNING_THRESHOLD = 5 # Log WARNING if >5 issues # Jira API limits diff --git a/connectors/jira/service.py b/connectors/jira/service.py index 45abd5b..3c300c3 100644 --- a/connectors/jira/service.py +++ b/connectors/jira/service.py @@ -23,6 +23,15 @@ from connectors.jira.validation import is_valid_issue_key, safe_join_under logger = logging.getLogger(__name__) +class JiraFetchError(Exception): + """Raised by Jira fetch helpers when the API returns an auth (401/403) + or server (5xx) error. Callers that overlay the result onto cached + issue JSON (save_issue, backfill processors) MUST catch this and + skip the overlay; otherwise a transient outage silently wipes + existing parquet rows for that issue. + """ + + class _JiraConfig: """Jira configuration from environment variables.""" JIRA_DOMAIN = os.environ.get("JIRA_DOMAIN", "") @@ -118,7 +127,7 @@ class JiraService: Fetch complete issue data from Jira. Args: - issue_key: Issue key (e.g., "KSP-123") + issue_key: Issue key (e.g., "PROJ-123") Returns: Issue data dict or None if fetch failed @@ -208,14 +217,28 @@ class JiraService: """ Fetch remote links for an issue from Jira. - Args: - issue_key: Issue key (e.g., "KSP-123") - - Returns: - List of remote link dicts, empty list on failure + Returns the list of remote links on 200; an empty list on 404 + (issue legitimately has no remote links). Raises JiraFetchError + on ANY other status code or on httpx.RequestError, so callers + that overlay this onto cached issue data can skip the overlay + instead of wiping existing rows. Critically, 429 rate limits + also raise — silently returning [] there would re-trigger the + same wipe bug (a webhook burst hitting Jira's rate limiter is + the most likely production scenario). """ + # Unconfigured-service case: per the new contract, callers + # interpret `[]` as "issue legitimately has no remote links" + # and `JiraFetchError` as "fetch failed, preserve existing + # rows". Silently returning `[]` here would overlay an empty + # list onto cached issue JSON and wipe existing parquet rows + # the next time a webhook fires while creds happen to be + # missing — the exact regression this PR closes for the 401 + # / 429 / 5xx paths. Raise instead so the overlay site skips. if not self.is_configured(): - return [] + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: " + "Jira service not configured (missing API credentials)" + ) url = f"{self.base_url}/issue/{issue_key}/remotelink" @@ -226,21 +249,34 @@ class JiraService: auth=self.auth, headers={"Accept": "application/json"}, ) - - if response.status_code == 200: - return response.json() - elif response.status_code == 404: - return [] - else: - logger.warning( - f"Failed to fetch remote links for {issue_key}: " - f"{response.status_code}" - ) - return [] - except httpx.RequestError as e: - logger.warning(f"Request error fetching remote links for {issue_key}: {e}") + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: connection — {e}" + ) from e + + if response.status_code == 200: + return response.json() + if response.status_code == 404: return [] + if response.status_code in (401, 403): + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: auth error " + f"({response.status_code}) — token may be expired/revoked" + ) + if response.status_code == 429: + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: rate limited " + f"(429) — retry later" + ) + if response.status_code >= 500: + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: server error " + f"({response.status_code})" + ) + raise JiraFetchError( + f"Remote-links fetch for {issue_key} failed: unexpected status " + f"{response.status_code}" + ) def save_issue(self, issue_data: dict[str, Any]) -> Path | None: """ @@ -276,10 +312,19 @@ class JiraService: # Add metadata issue_data["_synced_at"] = datetime.now(timezone.utc).isoformat() - # Fetch and embed remote links for Parquet transform + # Overlay-skip guard: if fetch_remote_links raises (auth/server failure), + # leave the _remote_links key ABSENT. transform_remote_links treats absent key + # as "no fresh data, preserve existing parquet rows". A present-but-empty list + # would be interpreted as "this issue has no remote links — wipe existing". issue_key_for_links = issue_data.get("key") if issue_key_for_links: - issue_data["_remote_links"] = self.fetch_remote_links(issue_key_for_links) + try: + issue_data["_remote_links"] = self.fetch_remote_links(issue_key_for_links) + except JiraFetchError as e: + logger.warning( + f"Skipping _remote_links overlay for {issue_key_for_links}: {e}. " + f"Existing parquet rows will be preserved." + ) # Overlay SLA fields from JSM service account (personal token lacks permissions) sla_fields = self.fetch_sla_fields(issue_key) diff --git a/connectors/jira/transform.py b/connectors/jira/transform.py index 70ce0b0..53ea88a 100644 --- a/connectors/jira/transform.py +++ b/connectors/jira/transform.py @@ -526,14 +526,32 @@ def transform_issuelinks(raw_issue: dict) -> list[dict]: return records -def transform_remote_links(raw_issue: dict) -> list[dict]: +def transform_remote_links(raw_issue: dict) -> list[dict] | None: """Extract and transform remote links from an issue. - Remote links are embedded in the raw issue JSON as `_remote_links` - by the fetch layer (jira_service.py / jira_backfill.py). + Returns: + - list[dict]: fresh records to upsert into parquet. May be empty, + meaning the issue legitimately has no remote links right now + (HTTP 200 with [] or HTTP 404 from the fetch). + - None: the _remote_links key was absent from raw_issue, which + signals that save_issue (or the equivalent backfill writer) + could not refresh remote links — typically a 401/403/5xx from + the Jira API. Callers MUST treat None as "skip the upsert"; + overwriting with [] would delete existing parquet rows for + this issue. + + The key shape is set by the writers (JiraService.save_issue, + JiraBackfiller, backfill_remote_links): present means the fetch + succeeded (200 or 404), absent means the fetch raised. """ issue_key = raw_issue.get("key") - remote_links = raw_issue.get("_remote_links", []) + # Treat both absent key and explicit None as the "no fresh data" signal — + # absent is the contract from save_issue/backfill writers, None is the + # defensive case where a JSON edit or older buggy code stored an explicit + # null (would otherwise blow up on `for rl in None`). + remote_links = raw_issue.get("_remote_links") + if remote_links is None: + return None records = [] for rl in remote_links: @@ -664,7 +682,15 @@ def transform_all( changelog_by_month[month_key].extend(transform_changelog(raw_issue)) issuelinks_by_month[month_key].extend(transform_issuelinks(raw_issue)) - remote_links_by_month[month_key].extend(transform_remote_links(raw_issue)) + rl_records = transform_remote_links(raw_issue) + if rl_records is not None: + remote_links_by_month[month_key].extend(rl_records) + # else: _remote_links overlay was skipped (fetch failure). The batch + # rebuild writes monthly parquets from scratch, so this issue simply + # contributes no rows to the rebuild — it doesn't "preserve" anything. + # A re-run after the outage clears will repopulate. The incremental + # path (incremental_transform.py) is what genuinely preserves + # existing rows; batch mode is full-rebuild and not the hot path. except Exception as e: logger.error(f"Error processing {json_file}: {e}") diff --git a/pyproject.toml b/pyproject.toml index 7644922..156e8d3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agnes-the-ai-analyst" -version = "0.54.18" +version = "0.54.19" description = "Agnes — AI Data Analyst platform for AI analytical systems" requires-python = ">=3.11,<3.14" license = "MIT" diff --git a/tests/test_jira_incremental.py b/tests/test_jira_incremental.py index 4bcc6d9..077371b 100644 --- a/tests/test_jira_incremental.py +++ b/tests/test_jira_incremental.py @@ -1,5 +1,6 @@ """Tests for incremental Jira parquet transform (upsert_dataframe and friends).""" +import json from pathlib import Path from unittest.mock import patch @@ -10,8 +11,10 @@ import pytest from connectors.jira.incremental_transform import ( load_parquet_month, save_parquet_month, + transform_single_issue, upsert_dataframe, ) +from connectors.jira.transform import REMOTE_LINKS_SCHEMA # Minimal schema compatible with ISSUES_SCHEMA for testing purposes @@ -183,3 +186,129 @@ class TestParquetMonthlyPartitioning: assert proj1.iloc[0]["summary"] == "Updated" proj2 = final[final["issue_key"] == "PROJ-2"] assert proj2.iloc[0]["summary"] == "Keep" + + +def _seed_remote_links_parquet(parquet_root, month_key, rows): + """Write a starter remote_links parquet so we can assert preservation.""" + df = pd.DataFrame(rows) + target = parquet_root / "remote_links" + target.mkdir(parents=True, exist_ok=True) + save_parquet_month(df, REMOTE_LINKS_SCHEMA, target, month_key) + + +def _write_raw_issue(raw_dir, issue_key, payload): + """Seed raw issue JSON at the path transform_single_issue reads from.""" + issues_dir = raw_dir / "issues" + issues_dir.mkdir(parents=True, exist_ok=True) + (issues_dir / f"{issue_key}.json").write_text(json.dumps(payload)) + + +def test_incremental_preserves_remote_links_when_overlay_absent(tmp_path): + """When the _remote_links key is absent from the raw JSON (the writer + skipped the overlay due to a Jira fetch failure), transform_single_issue + must NOT wipe existing parquet rows for that issue. Existing rows must + remain untouched and the function must report success.""" + raw_dir = tmp_path / "raw" + output_dir = tmp_path / "parquet" + attachments_dir = tmp_path / "attachments" + output_dir.mkdir() + attachments_dir.mkdir() + + # Pre-seed an existing remote-link row for PROJ-1 in month 2026-05. + _seed_remote_links_parquet(output_dir, "2026-05", [{ + "issue_key": "PROJ-1", + "remote_link_id": "rl-existing", + "url": "https://example.com/old", + "title": "Pre-existing link", + "application_name": "X", + "application_type": "x", + }]) + + # Raw issue WITHOUT _remote_links key — overlay was skipped upstream. + _write_raw_issue(raw_dir, "PROJ-1", { + "key": "PROJ-1", + "id": "10001", + "fields": { + "summary": "test", + "status": {"name": "Open"}, + "issuetype": {"name": "Bug"}, + "attachment": [], + "comment": {"comments": []}, + "created": "2026-05-15T00:00:00.000+0000", + "updated": "2026-05-15T00:00:00.000+0000", + }, + # NOTE: no _remote_links key — that is the test condition. + }) + + ok = transform_single_issue( + issue_key="PROJ-1", + raw_dir=raw_dir, + output_dir=output_dir, + attachments_dir=attachments_dir, + ) + assert ok is True + + df = load_parquet_month(output_dir / "remote_links", "2026-05") + assert df is not None and len(df) == 1, \ + "Existing remote-link row was wiped — overlay-absent signal not honored" + assert df.iloc[0]["remote_link_id"] == "rl-existing" + + +def test_incremental_wipes_remote_links_when_overlay_present_but_empty(tmp_path): + """The mirror-image case: when _remote_links IS present but the list is + empty, that's a successful fetch confirming the issue legitimately has no + remote links right now. The transform MUST wipe any existing parquet rows + for that issue — keeping them would be stale data. + + Together with test_incremental_preserves_remote_links_when_overlay_absent, + this locks the absent-vs-empty contract end-to-end. A future regression + that 'simplifies' the transform to treat [] the same as absent (i.e., + skip upsert) would be caught here.""" + raw_dir = tmp_path / "raw" + output_dir = tmp_path / "parquet" + attachments_dir = tmp_path / "attachments" + output_dir.mkdir() + attachments_dir.mkdir() + + # Pre-seed a stale row that should be wiped. + _seed_remote_links_parquet(output_dir, "2026-05", [{ + "issue_key": "PROJ-2", + "remote_link_id": "rl-stale", + "url": "https://example.com/stale", + "title": "Stale link to be wiped", + "application_name": "X", + "application_type": "x", + }]) + + # Raw issue WITH _remote_links: [] — fresh fetch confirmed empty. + _write_raw_issue(raw_dir, "PROJ-2", { + "key": "PROJ-2", + "id": "10002", + "fields": { + "summary": "test", + "status": {"name": "Open"}, + "issuetype": {"name": "Bug"}, + "attachment": [], + "comment": {"comments": []}, + "created": "2026-05-15T00:00:00.000+0000", + "updated": "2026-05-15T00:00:00.000+0000", + }, + "_remote_links": [], + }) + + ok = transform_single_issue( + issue_key="PROJ-2", + raw_dir=raw_dir, + output_dir=output_dir, + attachments_dir=attachments_dir, + ) + assert ok is True + + df = load_parquet_month(output_dir / "remote_links", "2026-05") + # Either the file was unlinked (df is None) or the row was removed. + # Both outcomes satisfy the contract — the stale row must not survive. + if df is not None: + remaining = df[df["issue_key"] == "PROJ-2"] + assert len(remaining) == 0, \ + "Stale remote-link row survived a successful empty-list fetch — " \ + "the empty-list (legitimate) signal was misinterpreted as preserve" diff --git a/tests/test_jira_service_full.py b/tests/test_jira_service_full.py index 82bc15b..a25471d 100644 --- a/tests/test_jira_service_full.py +++ b/tests/test_jira_service_full.py @@ -8,6 +8,10 @@ import pytest from tests.helpers.factories import WebhookEventFactory +from connectors.jira.scripts.consistency_check import JiraConsistencyChecker +from connectors.jira.service import JiraFetchError +from connectors.jira.transform import transform_remote_links + @pytest.fixture def jira_env(tmp_path, monkeypatch): @@ -180,3 +184,203 @@ class TestJiraServiceWebhookProcessing: secret.encode("utf-8"), payload, hashlib.sha256 ).hexdigest() assert sig == f"sha256={expected_mac}" + + +class TestFetchRemoteLinks: + """fetch_remote_links must raise on auth/server failure to prevent the + save_issue overlay from writing [] into cached JSON, which downstream + would interpret as 'delete existing remote_links rows for this issue'.""" + + def _mock_http(self, status_code, json_body=None): + """Build a MagicMock httpx.Client context manager returning a fixed response.""" + response = MagicMock() + response.status_code = status_code + response.json.return_value = json_body or [] + client = MagicMock() + client.get.return_value = response + client.__enter__ = lambda s: client + client.__exit__ = MagicMock(return_value=False) + return client + + def test_returns_list_on_200(self, jira_env): + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(200, [{"id": "1"}])): + assert service.fetch_remote_links("PROJ-1") == [{"id": "1"}] + + def test_returns_empty_on_404(self, jira_env): + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(404)): + assert service.fetch_remote_links("PROJ-1") == [] + + def test_raises_on_401(self, jira_env): + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(401)): + with pytest.raises(JiraFetchError, match="auth"): + service.fetch_remote_links("PROJ-1") + + def test_raises_on_403(self, jira_env): + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(403)): + with pytest.raises(JiraFetchError, match="auth"): + service.fetch_remote_links("PROJ-1") + + def test_raises_on_500(self, jira_env): + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(500)): + with pytest.raises(JiraFetchError, match="server"): + service.fetch_remote_links("PROJ-1") + + def test_raises_on_429_rate_limit(self, jira_env): + # 429 in the webhook hot path must raise (not silently return []). + # A webhook burst hitting Jira's rate limiter is the most likely + # production scenario; returning [] would re-trigger the wipe bug. + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(429)): + with pytest.raises(JiraFetchError, match="rate limited"): + service.fetch_remote_links("PROJ-1") + + def test_raises_on_unexpected_status(self, jira_env): + # Any non-success/non-404 status raises — covers 400, 405, 418, etc. + # No silent fall-through. + service = _make_jira_service(jira_env) + with patch("connectors.jira.service.httpx.Client", + return_value=self._mock_http(418)): + with pytest.raises(JiraFetchError, match="unexpected status"): + service.fetch_remote_links("PROJ-1") + + def test_raises_when_unconfigured(self, jira_env): + """Regression guard for the Devin adversarial-review finding: + when the Jira service is unconfigured (missing API credentials) + the fetch must raise JiraFetchError, NOT silently return []. + Silent [] would overlay an empty list onto cached issue JSON via + save_issue and wipe existing remote_links parquet rows the next + time a webhook fires while creds happen to be missing — the + exact scenario this PR closes for the 401 / 429 / 5xx paths. + The webhook hot path passes HMAC verification with + JIRA_WEBHOOK_SECRET (separate from API creds), so a + missing-creds + webhook-arrives combo is realistic, not + theoretical.""" + from connectors.jira import service as svc + # Mirror _make_jira_service except DROP the API credentials so + # is_configured() returns False. + svc.Config.JIRA_DOMAIN = "" + svc.Config.JIRA_EMAIL = "" + svc.Config.JIRA_API_TOKEN = "" + svc.Config.JIRA_DATA_DIR = jira_env + svc.Config.JIRA_WEBHOOK_SECRET = "webhook-secret-123" + svc._jira_service = None + service = svc.get_jira_service() + assert not service.is_configured() + with pytest.raises(JiraFetchError, match="not configured"): + service.fetch_remote_links("PROJ-1") + + def test_raises_on_request_error(self, jira_env): + import httpx + service = _make_jira_service(jira_env) + client = MagicMock() + client.get.side_effect = httpx.RequestError("connection reset") + client.__enter__ = lambda s: client + client.__exit__ = MagicMock(return_value=False) + with patch("connectors.jira.service.httpx.Client", return_value=client): + with pytest.raises(JiraFetchError, match="connection"): + service.fetch_remote_links("PROJ-1") + + +class TestSaveIssueRemoteLinksOverlay: + """save_issue must NOT set _remote_links when fetch_remote_links raises. + The absent key is the contract with transform_remote_links: it signals + 'preserve existing rows'. A present-but-empty list would wipe them.""" + + def test_sets_remote_links_on_success(self, jira_env): + service = _make_jira_service(jira_env) + with patch.object(service, "fetch_remote_links", return_value=[{"id": "rl-1"}]), \ + patch.object(service, "fetch_sla_fields", return_value=None): + path = service.save_issue(_fake_issue_data("PROJ-1")) + with open(path) as f: + data = json.load(f) + assert data["_remote_links"] == [{"id": "rl-1"}] + + def test_sets_empty_remote_links_on_404(self, jira_env): + # 404 stays as [] — legitimately means "issue has no remote links". + service = _make_jira_service(jira_env) + with patch.object(service, "fetch_remote_links", return_value=[]), \ + patch.object(service, "fetch_sla_fields", return_value=None): + path = service.save_issue(_fake_issue_data("PROJ-1")) + with open(path) as f: + data = json.load(f) + assert data["_remote_links"] == [] + + def test_omits_remote_links_key_on_fetch_error(self, jira_env): + # When fetch raises, the key MUST be absent — that's the signal + # to the transform that this isn't fresh data and should be skipped. + service = _make_jira_service(jira_env) + with patch.object(service, "fetch_remote_links", + side_effect=JiraFetchError("auth")), \ + patch.object(service, "fetch_sla_fields", return_value=None): + path = service.save_issue(_fake_issue_data("PROJ-1")) + with open(path) as f: + data = json.load(f) + assert "_remote_links" not in data, \ + "Absent key is the contract with transform_remote_links — " \ + "do not change to an empty list." + + +class TestTransformRemoteLinks: + """transform_remote_links returns None when _remote_links is absent + (preserve-existing signal) and [] when present-but-empty (legitimate 'none').""" + + def test_returns_list_when_links_present(self): + result = transform_remote_links({ + "key": "PROJ-1", + "_remote_links": [{ + "id": "rl-1", + "object": {"url": "https://x", "title": "X"}, + "application": {"name": "App", "type": "type"}, + }], + }) + assert result is not None + assert len(result) == 1 + assert result[0]["remote_link_id"] == "rl-1" + + def test_returns_empty_list_when_key_present_but_empty(self): + result = transform_remote_links({"key": "PROJ-1", "_remote_links": []}) + assert result == [] + + def test_returns_none_when_key_absent(self): + # Absent key = save_issue skipped the overlay because fetch failed. + # Signal to caller: preserve existing parquet rows for this issue. + result = transform_remote_links({"key": "PROJ-1"}) + assert result is None + + def test_returns_none_when_key_is_explicit_null(self): + # Defensive: a JSON with `_remote_links: null` (from older buggy code + # or a manual edit) would otherwise blow up on `for rl in None`. + # Treat null the same as absent — both mean "no fresh data". + result = transform_remote_links({"key": "PROJ-1", "_remote_links": None}) + assert result is None + + +class TestAutoFixThreshold: + """The auto-fix threshold determines at what gap size we auto-backfill + vs require manual review. Legacy bumped it from 10 to 20 — small enough + to stay safe, big enough to absorb a typical SLA-poller hiccup.""" + + def test_threshold_is_twenty(self): + assert JiraConsistencyChecker.AUTO_FIX_THRESHOLD == 20 + + def test_alert_level_warning_at_threshold(self): + # 20 missing should still be WARNING (auto-fix territory), not ERROR. + config = MagicMock() + checker = JiraConsistencyChecker(config) + assert checker.get_alert_level(20) == "WARNING" + + def test_alert_level_error_above_threshold(self): + config = MagicMock() + checker = JiraConsistencyChecker(config) + assert checker.get_alert_level(21) == "ERROR"