diff --git a/agents/IMPLEMENTATION.md b/agents/IMPLEMENTATION.md index 24730f6..2f75b3e 100644 --- a/agents/IMPLEMENTATION.md +++ b/agents/IMPLEMENTATION.md @@ -30,6 +30,8 @@ ## Recently Completed (June 2026) +- **Quickstart P10Y repository lookup pagination (Jul 8)**: Fixed Quickstart initialization for P10Y organizations with more than one repository page. `create_generation_session_repos.py` now searches generated repos with the qualified GitHub owner/prefix (for example `org/specflow-workspace`) and uses a shared paginated P10Y repository listing helper for ID resolution, re-fetch connection discovery, status checks, and status polling. Tests: `uv run pytest test/scripts/test_create_generation_session_repos.py -q` from `backend` (25 passed). + - **TUI workspace notification dedupe (Jul 1)**: Fixed workspace drill-in causing repeated desktop "Workspace phase progressed" notifications every poll during KB init. `MilestoneTracker` now emits workspace progress only when `last_completed_phase` increases, so label-only differences between session-list and `/status` payloads stay silent. `WorkspaceMessagesScreen` feeds its `/status` poll into the shared per-run tracker, and the app-wide active-session watcher excludes the workspace screen's generation to avoid mixed payload sources. Tests: `test_tui_poller.py::TestMilestoneTracker::test_kb_init_phase_label_change_without_progress_is_silent`, `test_tui_app.py::TestWorkspaceDrillIn::test_workspace_screen_excludes_app_level_session_watcher`; broader TUI check: `uv run pytest tests/test_tui_app.py tests/test_tui_poller.py -v` from `mcp_server` (78 passed). - **MCP client setup screen in the TUI (Jun 30)**: New `ClientSetupScreen` (`mcp_server/tui/app.py`) + pure registry `mcp_server/tui/mcp_clients.py` that connect SpecFlow's local MCP server to the user's AI tool, replacing the post-onboarding "No active generation sessions." dead-end (shown first-run via `_startup_gate`, re-openable with `c` on Sessions/Dashboard). **Connect UX**: auto-detects installed clients (`shutil.which` + `~/.cursor`), one-key connect, honest per-client status (green = connected/verified, amber = added-can't-verify, red = failed, grey = not-connected / copy-only). Claude Code → `claude mcp remove`+`add-json -s user` then verify `claude mcp get`; Gemini → `gemini mcp add … -s user` (silent overwrite, trust-folder caveat); Cursor → merge `~/.cursor/mcp.json` (refuse-on-malformed, `.bak` only after confirm) + best-effort deeplink; universal "copy config" fallback. Cursor/Gemini have no read-back so they cap at "added — confirm in your client"; revisiting opens a Yes/No inspection that promotes to connected or demotes to failed. **Real per-client status persisted** in the global **`~/.specflow/config.json`** (`clients` section, values = TUI statuses) so an unverified add is never assumed connected across sessions — global because connecting a client is a machine-wide act (`-s user` / `~/.cursor`) and the TUI must reach it from any project. **`~/.specflow/config.json` is the SSOT for future global SpecFlow/MCP settings** — add new sections as sibling top-level keys via `mcp_clients._read_config`/`_write_config` (they preserve unknown keys); do not put global settings in the project-local `.specflow-local/`. **UI follow-ups (Jul):** modals use the app `Footer` ("ESC to close") instead of an inline hint; the works/doesn't-work inspection is an arrow-navigable `ListView` (not buttons); and on screen open a background probe runs `claude mcp get specflow` for verifiable clients — an already-registered server shows **verified without re-adding** (helps users who already have it), and a stale saved "connected" is cleared (`mcp_clients.forget_status`) if it's gone. One registry feeds both the screen and the `init` CLI hint (replaced the hardcoded `_IDE_REGISTRATION_HINT`). **Hang-safe `local_env.run_command`** (timeout + `proc.kill()` + reap) added because verify/probe commands like `claude mcp get` can block on a network socket with no output — the existing `_stream_subprocess` does an unbounded `await proc.wait()` and would freeze the exclusive Textual worker with no escape. Tests: `test_tui_mcp_clients.py` (pure: json forms, Gemini flag translation, deeplink encoding+no-raw-`+`, merge preserves siblings, status/marker), `test_tui_app.py::TestClientSetupScreen` (render/preselect, connect→verified/added-unverified, malformed-refuse-untouched, inspect confirm/reject/later, status persistence), `test_local_env.py::TestRunCommand` (timeout kills hung process, no zombie). 542 unit tests pass. diff --git a/agents/MEMORY.md b/agents/MEMORY.md index 9b7a8cc..a205dbe 100644 --- a/agents/MEMORY.md +++ b/agents/MEMORY.md @@ -10,3 +10,4 @@ Concise, generalized lessons (not a changelog — that is `agents/IMPLEMENTATION - Firestore emulator imports require the `*overall_export_metadata` file path; export destinations are directories, but `--import-data` must not point at the snapshot directory itself. - Global SpecFlow/TUI settings live in `~/.specflow/config.json` (SSOT) — read/write via `mcp_server/tui/mcp_clients.py` `_read_config`/`_write_config`, which preserve unknown top-level keys; add each new setting as its own top-level section. Do NOT put global settings in the project-local `.specflow-local/` (that dir is per-project runtime: `mcp-config.json`, `workspaces.json`, `init.log`). MCP-client connection status is stored globally because connecting a client is a machine-wide act (`claude/gemini mcp add -s user`, Cursor `~/.cursor/mcp.json`). - Run MCP server pytest commands from `mcp_server/` (`uv run pytest tests/...`); root-level `uv run pytest ...` may not expose a pytest executable because the repo has per-package `pyproject.toml` environments. +- P10Y repository list reads in Quickstart must use the narrowest available search (`github_org/prefix` for generated repos) and paginate; first-page org scans miss repos in large Compass organizations. diff --git a/backend/app/services/p10y/p10y_api_client.py b/backend/app/services/p10y/p10y_api_client.py index f054193..bce2391 100644 --- a/backend/app/services/p10y/p10y_api_client.py +++ b/backend/app/services/p10y/p10y_api_client.py @@ -19,6 +19,22 @@ logging.getLogger("httpx").setLevel(logging.INFO) logging.getLogger("httpcore").setLevel(logging.INFO) +_REPOSITORY_LIST_PAGE_SIZE = 1000 +_REPOSITORY_LIST_MAX_PAGES = 1000 + + +def _response_total_pages(response: Dict[str, Any]) -> Optional[int]: + """Extract a positive page count from a paginated P10Y response, if present.""" + total_pages = response.get("totalPages") or response.get("total_pages") + if isinstance(total_pages, bool) or total_pages is None: + return None + try: + parsed = int(total_pages) + except (TypeError, ValueError): + return None + return parsed if parsed > 0 else None + + class P10YInternalAPIClient: """ Client for interacting with the P10Y Internal API. @@ -220,6 +236,40 @@ async def list_repositories( ) return response.json() + async def list_repositories_paginated( + self, + organisation_id: int, + search: Optional[str] = None, + page_size: int = _REPOSITORY_LIST_PAGE_SIZE, + max_pages: int = _REPOSITORY_LIST_MAX_PAGES, + ) -> List[Dict[str, Any]]: + """Read all repository pages within an organization for the given search scope.""" + repositories: List[Dict[str, Any]] = [] + page = 1 + + while True: + repos_response = await self.list_repositories( + organisation_id=organisation_id, + search=search, + page=page, + page_size=page_size, + ) + page_data = repos_response.get("data", []) + repositories.extend(page_data) + + total_pages = _response_total_pages(repos_response) + if total_pages is not None: + if page >= total_pages: + break + elif len(page_data) < page_size: + break + + page += 1 + if page > max_pages: + raise RuntimeError(f"P10Y repository listing exceeded {max_pages} pages") + + return repositories + async def update_repository( self, organisation_id: int, diff --git a/backend/scripts/create_generation_session_repos.py b/backend/scripts/create_generation_session_repos.py index f227eec..a5a3a2a 100755 --- a/backend/scripts/create_generation_session_repos.py +++ b/backend/scripts/create_generation_session_repos.py @@ -331,11 +331,20 @@ def _normalize_git_url(git_url: str) -> str: return s +def _p10y_repository_search(prefix: str, github_org: Optional[str]) -> str: + """Build the narrowest P10Y repository search value available.""" + clean_prefix = (prefix or "").strip() + clean_org = (github_org or "").strip().strip("/") + if clean_prefix and clean_org: + return f"{clean_org}/{clean_prefix}" + return clean_prefix + + async def get_repository_ids( p10y_client: P10YInternalAPIClient, org_id: int, repo_names: List[str], - prefix: str, + search: Optional[str] = None, github_org: Optional[str] = None, ) -> Dict[str, int]: """ @@ -345,7 +354,8 @@ async def get_repository_ids( p10y_client: Initialized P10y API client org_id: P10y organization ID repo_names: List of repository names to find - prefix: Search prefix for list_repositories + search: Search filter for list_repositories (e.g. the qualified + ``/`` string built by ``_p10y_repository_search``) github_org: GitHub org owning the repos. When set, matching is done on ``git_url`` (``/``) rather than the bare ``repository_name``. Returns: @@ -353,12 +363,7 @@ async def get_repository_ids( """ print("\n🔍 Looking up P10y repository IDs") - # Fetch all repositories - repos_response = await p10y_client.list_repositories( - organisation_id=org_id, - search=prefix, - page_size=1000, # Should be enough for our case, - ) + repos = await p10y_client.list_repositories_paginated(org_id, search=search) # P10Y `repository_name` is the BARE repo name and is NOT unique within a Compass # organisation — the same bare name can exist under several GitHub orgs, distinguished @@ -373,7 +378,7 @@ async def get_repository_ids( repo_name_set = set(repo_names) repo_id_map: Dict[str, int] = {} - for repo_data in repos_response.get("data", []): + for repo_data in repos: if expected_by_git_url: matched_name = expected_by_git_url.get( _normalize_git_url(repo_data.get("git_url", "")) @@ -402,16 +407,24 @@ async def trigger_repository_refetch( p10y_client: P10YInternalAPIClient, org_id: int, github_org: Optional[str], - repo_names: List[str], ) -> None: - """Trigger Compass's 'Re-fetch' on the connection(s) owning the workspace repos; omitting connection_id may return 400 but the sync still completes.""" - repos = (await p10y_client.list_repositories(organisation_id=org_id, page_size=1000)).get("data", []) - expected = ( - {_normalize_git_url(f"{github_org}/{name}") for name in repo_names} if github_org else None - ) + """Trigger Compass's re-fetch on the connection(s) owning ``github_org``. + + A Compass connection is per GitHub org/account, not per repo, so any repo + already ingested under ``github_org`` reveals the right connection — the + brand-new repos being provisioned are never yet visible in P10Y (that's + why this is being called), so matching only their exact names would + always miss and force a broadcast re-fetch across every active GitHub + connection instead of just the one that actually owns them. + """ + search = github_org.strip().strip("/") if github_org else None + repos = await p10y_client.list_repositories_paginated(org_id, search=search) + org_prefix = f"{_normalize_git_url(github_org)}/" if github_org else None + conn_ids: set[int] = set() for repo_data in repos: - if expected is not None and _normalize_git_url(repo_data.get("git_url", "")) not in expected: + git_url = _normalize_git_url(repo_data.get("git_url", "")) + if org_prefix is not None and not git_url.startswith(org_prefix): continue cid = (repo_data.get("_embedded", {}).get("connection") or {}).get("id_connection") if cid: @@ -450,19 +463,17 @@ async def get_repository_statuses( p10y_client: P10YInternalAPIClient, org_id: int, repo_ids: List[int], + search: Optional[str] = None, ) -> Dict[int, Dict[str, Any]]: """Fetch current P10Y statuses for the target repository IDs.""" if not repo_ids: return {} - repos_response = await p10y_client.list_repositories( - organisation_id=org_id, - page_size=1000, - ) + repos = await p10y_client.list_repositories_paginated(org_id, search=search) target_ids = set(repo_ids) statuses: Dict[int, Dict[str, Any]] = {} - for repo_data in repos_response.get("data", []): + for repo_data in repos: repo_id = _p10y_repository_id(repo_data) if repo_id in target_ids: statuses[repo_id] = { @@ -521,7 +532,8 @@ async def poll_repository_status( org_id: int, repo_ids: List[int], timeout_minutes: int = 5, - poll_interval: int = 15 + poll_interval: int = 15, + search: Optional[str] = None, ) -> Dict[int, Dict[str, Any]]: """ Poll P10y to check when repositories become live with metrics. @@ -555,13 +567,10 @@ async def poll_repository_status( try: # Fetch repository details - repos_response = await p10y_client.list_repositories( - organisation_id=org_id, - page_size=1000 - ) + repos = await p10y_client.list_repositories_paginated(org_id, search=search) # Update status for our repos - for repo_data in repos_response.get("data", []): + for repo_data in repos: repo_id = _p10y_repository_id(repo_data) if repo_id in repo_ids: internal_status = repo_data.get("internal_status") @@ -591,7 +600,11 @@ async def poll_repository_status( # Wait before next poll await asyncio.sleep(poll_interval) - + + except RuntimeError: + # Structural failure (e.g. pagination cap exceeded) — not a transient + # polling error, so fail fast instead of retrying until timeout. + raise except Exception as e: print(f"⚠️ Error polling status: {e}") await asyncio.sleep(poll_interval) @@ -953,7 +966,10 @@ async def main(): firestore_target_from_cli = bool(gcp_cli and fsdb_cli) # Validate arguments - if args.start > args.end: + if args.repos is None and (args.start is None or args.end is None): + parser.error("--start and --end are required unless --repos is provided") + + if args.start is not None and args.end is not None and args.start > args.end: print("❌ Error: start number must be less than or equal to end number") sys.exit(1) @@ -1038,9 +1054,6 @@ async def main(): elif not git_username: git_username = "(not resolved; set GIT_USER_NAME_DEFAULT or GITHUB_TOKEN_DEFAULT)" - if args.repos is None and (args.start is None or args.end is None): - parser.error("--start and --end are required unless --repos is provided") - print("=" * 80) title = "🚀 Generation Workspace Repository Setup" if args.dry_run: @@ -1113,8 +1126,12 @@ async def main(): repo_names = [f"{args.prefix}{num}" for num in range(args.start, args.end + 1)] # Step 2: Get P10y repository IDs - # For --repos, search with empty prefix to match arbitrary names across the full org. - p10y_search_prefix = "" if own_repo_list is not None else args.prefix + # For --repos, search with an empty string to match arbitrary names across the full org. + # Computed once and reused verbatim (never re-qualified) by every lookup below — + # get_repository_ids no longer builds its own search string from a raw prefix. + p10y_search_prefix = ( + "" if own_repo_list is not None else _p10y_repository_search(args.prefix, github_org) + ) repo_id_map = await get_repository_ids( p10y_client, p10y_org_id, repo_names, p10y_search_prefix, github_org ) @@ -1126,7 +1143,7 @@ async def main(): missing = [r for r in repo_names if r not in repo_id_map] if missing: print(f"\n🔄 {len(missing)} repo(s) not in P10Y yet: {', '.join(missing)} — triggering re-fetch ...") - await trigger_repository_refetch(p10y_client, p10y_org_id, github_org, repo_names) + await trigger_repository_refetch(p10y_client, p10y_org_id, github_org) deadline = time.time() + P10Y_REFETCH_TIMEOUT_SECONDS while missing and time.time() < deadline: @@ -1157,7 +1174,12 @@ async def main(): # Step 4: Start metrics calculation only for repos that are not already Live. # The --repos path skips metrics: those repos already have history in Compass. if not args.skip_metrics and own_repo_list is None: - current_statuses = await get_repository_statuses(p10y_client, p10y_org_id, repo_ids) + current_statuses = await get_repository_statuses( + p10y_client, + p10y_org_id, + repo_ids, + search=p10y_search_prefix, + ) metrics_repo_ids = repository_ids_requiring_metrics(repo_ids, current_statuses) if metrics_repo_ids: @@ -1172,7 +1194,8 @@ async def main(): p10y_org_id, repo_ids, args.poll_timeout, - args.poll_interval + args.poll_interval, + search=p10y_search_prefix, ) else: final_statuses = current_statuses diff --git a/backend/test/scripts/test_create_generation_session_repos.py b/backend/test/scripts/test_create_generation_session_repos.py index 50a0368..f809201 100644 --- a/backend/test/scripts/test_create_generation_session_repos.py +++ b/backend/test/scripts/test_create_generation_session_repos.py @@ -21,6 +21,7 @@ import pytest import scripts.create_generation_session_repos as cgsr +from app.services.p10y.p10y_api_client import P10YInternalAPIClient from scripts.init_firestore import WorkspaceConfig # --------------------------------------------------------------------------- @@ -31,6 +32,12 @@ _MAKEFILE = _BACKEND_DIR.parent / "Makefile" +def _make_p10y_client() -> P10YInternalAPIClient: + """A real client so list_repositories_paginated runs its actual pagination loop + against a mocked list_repositories, instead of being auto-mocked away.""" + return P10YInternalAPIClient(base_url="https://p10y.test") + + # =========================================================================== # (a) emit_workspace_config — schema parity with WorkspaceConfig # =========================================================================== @@ -181,14 +188,14 @@ class TestGetRepositoryIdsNoProjectFilter: @pytest.mark.asyncio async def test_list_repositories_called_without_project_ids(self): """get_repository_ids must not pass project_ids to list_repositories.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={"data": []}) await cgsr.get_repository_ids( p10y_client=mock_client, org_id=42, repo_names=["specflow-workspace1"], - prefix="specflow-workspace", + search="specflow-workspace", ) mock_client.list_repositories.assert_called_once() @@ -197,10 +204,59 @@ async def test_list_repositories_called_without_project_ids(self): "get_repository_ids must not pass project_ids to list_repositories" ) + @pytest.mark.asyncio + async def test_list_repositories_forwards_qualified_search_verbatim(self): + """get_repository_ids forwards the caller-supplied search string as-is (it must not + re-qualify it — that double-qualifies when a caller passes an already-qualified + value, e.g. main()'s post-refetch retry loop).""" + mock_client = _make_p10y_client() + mock_client.list_repositories = AsyncMock(return_value={"data": []}) + + qualified_search = cgsr._p10y_repository_search("specflow-workspace", "myorg") + + await cgsr.get_repository_ids( + p10y_client=mock_client, + org_id=42, + repo_names=["specflow-workspace1"], + search=qualified_search, + github_org="myorg", + ) + + call_kwargs = mock_client.list_repositories.call_args.kwargs + assert call_kwargs["search"] == "myorg/specflow-workspace" + + @pytest.mark.asyncio + async def test_repeated_calls_do_not_double_qualify_search(self): + """Calling get_repository_ids twice with the same pre-qualified search (as main()'s + initial lookup and its post-refetch retry loop both do) must send the identical + search string both times, not a progressively re-qualified one.""" + mock_client = _make_p10y_client() + mock_client.list_repositories = AsyncMock(return_value={"data": []}) + + qualified_search = cgsr._p10y_repository_search("specflow-workspace", "myorg") + + await cgsr.get_repository_ids( + p10y_client=mock_client, + org_id=42, + repo_names=["specflow-workspace1"], + search=qualified_search, + github_org="myorg", + ) + await cgsr.get_repository_ids( + p10y_client=mock_client, + org_id=42, + repo_names=["specflow-workspace1"], + search=qualified_search, + github_org="myorg", + ) + + searches = [c.kwargs["search"] for c in mock_client.list_repositories.call_args_list] + assert searches == ["myorg/specflow-workspace", "myorg/specflow-workspace"] + @pytest.mark.asyncio async def test_list_repositories_returns_repo_mapping(self): """get_repository_ids returns a mapping of repo_name -> id.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={ "data": [ {"repository_name": "specflow-workspace1", "id": 55555}, @@ -212,11 +268,50 @@ async def test_list_repositories_returns_repo_mapping(self): p10y_client=mock_client, org_id=42, repo_names=["specflow-workspace1", "specflow-workspace2"], - prefix="specflow-workspace", + search="specflow-workspace", ) assert result == {"specflow-workspace1": 55555, "specflow-workspace2": 55556} + @pytest.mark.asyncio + async def test_list_repositories_paginates_until_target_repo_found(self): + """Repository lookup must continue past the first P10Y page.""" + mock_client = _make_p10y_client() + mock_client.list_repositories = AsyncMock(side_effect=[ + { + "data": [ + { + "repository_name": "specflow-workspace1", + "git_url": "https://github.com/myorg/specflow-workspace1", + "id": 55555, + } + ], + "totalPages": 2, + }, + { + "data": [ + { + "repository_name": "specflow-workspace1001", + "git_url": "https://github.com/myorg/specflow-workspace1001", + "id": 56555, + } + ], + "totalPages": 2, + }, + ]) + + result = await cgsr.get_repository_ids( + p10y_client=mock_client, + org_id=42, + repo_names=["specflow-workspace1001"], + search="myorg/specflow-workspace", + github_org="myorg", + ) + + assert result == {"specflow-workspace1001": 56555} + pages = [call.kwargs["page"] for call in mock_client.list_repositories.await_args_list] + assert pages == [1, 2] + # =========================================================================== # (b2) trigger_repository_refetch — scope the re-fetch to the owning connection @@ -230,7 +325,7 @@ class TestTriggerRepositoryRefetch: @pytest.mark.asyncio async def test_uses_connection_id_of_matching_repo(self): """The connection id is taken from an already-ingested repo matched by git_url.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={ "data": [ { @@ -240,21 +335,44 @@ async def test_uses_connection_id_of_matching_repo(self): }, ] }) + mock_client.list_connections = AsyncMock() + mock_client.sync_repositories = AsyncMock() - await cgsr.trigger_repository_refetch( - mock_client, - org_id=42, - github_org="myorg", - repo_names=["specflow-workspace1", "specflow-workspace2"], - ) + await cgsr.trigger_repository_refetch(mock_client, org_id=42, github_org="myorg") + + mock_client.sync_repositories.assert_awaited_once_with(42, connection_id=555) + mock_client.list_connections.assert_not_called() + + @pytest.mark.asyncio + async def test_matches_any_repo_under_same_org_not_just_the_new_ones(self): + """The brand-new repos being provisioned are never yet visible in P10Y — that's + why this is called — so the connection must be discoverable from ANY already + ingested repo under the same GitHub org/account, not just an exact match to the + new repo names, or every real-world call would fall through to broadcasting the + re-fetch across every active GitHub connection.""" + mock_client = _make_p10y_client() + mock_client.list_repositories = AsyncMock(return_value={ + "data": [ + { + "repository_name": "some-unrelated-older-repo", + "git_url": "https://github.com/myorg/some-unrelated-older-repo", + "_embedded": {"connection": {"id_connection": 555}}, + }, + ] + }) + mock_client.list_connections = AsyncMock() + mock_client.sync_repositories = AsyncMock() + + await cgsr.trigger_repository_refetch(mock_client, org_id=42, github_org="myorg") mock_client.sync_repositories.assert_awaited_once_with(42, connection_id=555) mock_client.list_connections.assert_not_called() + assert mock_client.list_repositories.call_args.kwargs["search"] == "myorg" @pytest.mark.asyncio async def test_falls_back_to_active_github_connections(self): """When no repo matches, sync every active GitHub connection.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={"data": []}) mock_client.list_connections = AsyncMock(return_value={ "data": [ @@ -263,20 +381,16 @@ async def test_falls_back_to_active_github_connections(self): {"connection_id": 999, "connection_type": "github", "connection_status": "inactive"}, ] }) + mock_client.sync_repositories = AsyncMock() - await cgsr.trigger_repository_refetch( - mock_client, - org_id=42, - github_org="myorg", - repo_names=["specflow-workspace1"], - ) + await cgsr.trigger_repository_refetch(mock_client, org_id=42, github_org="myorg") mock_client.sync_repositories.assert_awaited_once_with(42, connection_id=777) @pytest.mark.asyncio async def test_no_op_when_no_active_github_connections(self): """When list_connections returns no active GitHub connections, skip sync and return.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={"data": []}) mock_client.list_connections = AsyncMock(return_value={ "data": [ @@ -284,13 +398,9 @@ async def test_no_op_when_no_active_github_connections(self): {"connection_id": 222, "connection_type": "github", "connection_status": "inactive"}, ] }) + mock_client.sync_repositories = AsyncMock() - await cgsr.trigger_repository_refetch( - mock_client, - org_id=42, - github_org="myorg", - repo_names=["specflow-workspace1"], - ) + await cgsr.trigger_repository_refetch(mock_client, org_id=42, github_org="myorg") mock_client.sync_repositories.assert_not_called() @@ -305,7 +415,7 @@ class TestP10YMetricsStatusGate: @pytest.mark.asyncio async def test_get_repository_statuses_accepts_id_field(self): """Status lookup accepts the list_repositories id field used by ID lookup.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={ "data": [ { @@ -331,7 +441,7 @@ async def test_get_repository_statuses_accepts_id_field(self): @pytest.mark.asyncio async def test_get_repository_statuses_accepts_id_repository_field(self): """Status lookup also accepts id_repository from P10Y status responses.""" - mock_client = AsyncMock() + mock_client = _make_p10y_client() mock_client.list_repositories = AsyncMock(return_value={ "data": [ { @@ -348,6 +458,36 @@ async def test_get_repository_statuses_accepts_id_repository_field(self): assert result[201]["repo_name"] == "specflow-workspace1" assert result[201]["status"] == "Live" + @pytest.mark.asyncio + async def test_get_repository_statuses_paginates(self): + """Status lookup must not miss target repo IDs beyond the first P10Y page.""" + mock_client = _make_p10y_client() + mock_client.list_repositories = AsyncMock(side_effect=[ + {"data": [{"id": 101, "repository_name": "specflow-workspace1"}], "totalPages": 2}, + { + "data": [ + { + "id": 102, + "repository_name": "specflow-workspace2", + "status": "Live", + } + ], + "totalPages": 2, + }, + ]) + + result = await cgsr.get_repository_statuses( + mock_client, + 42, + [102], + search="myorg/specflow-workspace", + ) + + assert result[102]["repo_name"] == "specflow-workspace2" + assert result[102]["status"] == "Live" + pages = [call.kwargs["page"] for call in mock_client.list_repositories.await_args_list] + assert pages == [1, 2] + def test_repository_ids_requiring_metrics_skips_live_repos(self): """Only repositories not already Live should be passed to enable_metrics.""" repo_statuses = { @@ -361,6 +501,33 @@ def test_repository_ids_requiring_metrics_skips_live_repos(self): assert result == [2, 3, 4] +# =========================================================================== +# (c2) poll_repository_status — a pagination-cap RuntimeError must fail fast, +# not be treated as a transient polling error and retried until timeout. +# =========================================================================== + +class TestPollRepositoryStatusFailsFastOnRuntimeError: + @pytest.mark.asyncio + async def test_runtime_error_propagates_instead_of_being_retried(self): + """A RuntimeError from list_repositories_paginated (e.g. pagination cap exceeded) + must propagate immediately, not be swallowed by the generic transient-error handler.""" + mock_client = _make_p10y_client() + mock_client.list_repositories_paginated = AsyncMock( + side_effect=RuntimeError("P10Y repository listing exceeded 1000 pages") + ) + + with pytest.raises(RuntimeError, match="exceeded 1000 pages"): + await cgsr.poll_repository_status( + mock_client, + org_id=42, + repo_ids=[101], + timeout_minutes=5, + poll_interval=0, + ) + + mock_client.list_repositories_paginated.assert_awaited_once() + + # =========================================================================== # (d) add_repositories_to_project is not callable from the module # =========================================================================== diff --git a/backend/test/services/test_p10y_api_client.py b/backend/test/services/test_p10y_api_client.py index 7e84164..414f7d2 100644 --- a/backend/test/services/test_p10y_api_client.py +++ b/backend/test/services/test_p10y_api_client.py @@ -63,3 +63,64 @@ async def test_sync_repositories_includes_connection_id_when_provided() -> None: client._make_request.assert_awaited_once_with( "POST", "/api/organisation/42/repository/sync", json_data={"connection_id": 7} ) + + +class TestListRepositoriesPaginated: + """list_repositories_paginated() walks every page of list_repositories().""" + + @pytest.mark.asyncio + async def test_single_page_via_total_pages(self) -> None: + """totalPages == 1 stops after the first request.""" + client = P10YInternalAPIClient(base_url="https://p10y.test") + client.list_repositories = AsyncMock( + return_value={"data": [{"id": 1}, {"id": 2}], "totalPages": 1} + ) + + result = await client.list_repositories_paginated(42, search="myorg/prefix") + + assert result == [{"id": 1}, {"id": 2}] + client.list_repositories.assert_awaited_once_with( + organisation_id=42, search="myorg/prefix", page=1, page_size=1000 + ) + + @pytest.mark.asyncio + async def test_walks_multiple_pages_via_total_pages(self) -> None: + """Keeps requesting pages until page >= totalPages.""" + client = P10YInternalAPIClient(base_url="https://p10y.test") + client.list_repositories = AsyncMock( + side_effect=[ + {"data": [{"id": 1}], "totalPages": 2}, + {"data": [{"id": 2}], "totalPages": 2}, + ] + ) + + result = await client.list_repositories_paginated(42) + + assert result == [{"id": 1}, {"id": 2}] + pages = [c.kwargs["page"] for c in client.list_repositories.await_args_list] + assert pages == [1, 2] + + @pytest.mark.asyncio + async def test_falls_back_to_short_page_when_total_pages_missing(self) -> None: + """Without totalPages, a page shorter than page_size ends pagination.""" + client = P10YInternalAPIClient(base_url="https://p10y.test") + client.list_repositories = AsyncMock( + side_effect=[ + {"data": [{"id": 1}, {"id": 2}]}, + {"data": [{"id": 3}]}, + ] + ) + + result = await client.list_repositories_paginated(42, page_size=2) + + assert result == [{"id": 1}, {"id": 2}, {"id": 3}] + assert client.list_repositories.await_count == 2 + + @pytest.mark.asyncio + async def test_raises_when_max_pages_exceeded(self) -> None: + """A runaway pagination loop (no totalPages, always a full page) fails loudly.""" + client = P10YInternalAPIClient(base_url="https://p10y.test") + client.list_repositories = AsyncMock(return_value={"data": [{"id": 1}]}) + + with pytest.raises(RuntimeError, match="exceeded 2 pages"): + await client.list_repositories_paginated(42, page_size=1, max_pages=2)