Fix orchestration build search preview mode and pagination

[anjor]

Summary

• force on orchestration build search calls
• fix paginated build search response shape so pagination collects build results correctly
• add service-level regression tests for previewed search (single page and paginated)

Testing

• uv run pytest tests/test_services/test_orchestration.py
• uv run pytest tests/test_commands/test_orchestration.py

Closes #147

[claude]

Code Review

Good targeted fix for issue #147. The PR addresses two distinct bugs — the missing preview=True parameter and the broken pagination response shape. Overall the approach is sound; a few observations below.

---

What's fixed

1. Response shape bug (most important) — src/pltr/services/orchestration.py:194-198

Before this PR, fetch_page returned _format_builds_search_response(response) directly, which has shape {"builds": [...], "next_page_token": ...}. But ResponsePaginationHandler.collect_pages() in pagination.py:157 does response.get("data", []), so it was always collecting an empty list on every page. The fix correctly remaps the keys:

return {
    "data": formatted.get("builds", []),
    "next_page_token": formatted.get("next_page_token"),
}

This was the silent data-loss bug, and it's fixed correctly.

2. preview=True added to both code paths — correct.

---

Concerns

Style inconsistency in search_builds() (orchestration.py:150-151)

The dict is initialized empty and then mutated on the next line, while search_builds_paginated() initialises preview inline. Prefer the inline style for consistency:

# Current (inconsistent)
kwargs: Dict[str, Any] = {}
kwargs["preview"] = True

# Better — matches search_builds_paginated style
kwargs: Dict[str, Any] = {"preview": True}

Root cause left open

Issue #147 identifies the root cause as FoundryClient being created without preview=True in auth/token.py (and likely auth/oauth.py). This PR patches only the build search code paths. Any other orchestration endpoint that requires preview mode will hit the same ApiFeaturePreviewUsageOnly error. A follow-up to fix the client-level preview=True (or a comment explaining the deliberate choice to keep it call-site scoped) would be helpful.

Test gap — multi-page pagination

test_search_builds_paginated_uses_preview uses max_pages=1 and next_page_token = None, which doesn't exercise the key fix (the "data"" vs "builds"` key remapping). A test with two pages would confirm that pagination actually collects items across pages now:

def test_search_builds_paginated_collects_multiple_pages(...):
    # page 1 returns next_page_token, page 2 returns None
    # assert len(result.data) == 2  (one build per page)
    # assert mock_build_class.search.call_count == 2

This would give confidence that the shape fix is exercised end-to-end.

---

Minor

The new assertion added to the pre-existing test_search_builds_success is a good strengthening of that test — no issues there.

---

Overall: the bug fixes are correct and the added tests are a good start. The three items above (style nit, root-cause tracking, multi-page test) are worth addressing before merge, with the multi-page test being the most valuable.

[claude]

PR Review: Fix orchestration build search preview mode and pagination

Good set of fixes here — the core pagination bug is real and the test coverage is a nice addition. A few things worth discussing before merging:

---

Core pagination bug fix (correct)

The root cause fix in search_builds_paginated is correct. _paginate_response / ResponsePaginationHandler.collect_pages expects the fetch_fn to return {"data": [...], "next_page_token": ...}, but previously fetch_page was returning _format_builds_search_response()'s output which uses a "builds" key. That would cause all paginated results to silently return empty lists. Good catch.

---

Issue: TypeError catch scope is too broad

In oauth.py and token.py:

try:
    return FoundryClient(auth=auth, hostname=self.host, preview=True)
except TypeError:
    return FoundryClient(auth=auth, hostname=self.host)

A TypeError thrown here isn't necessarily about the preview kwarg — it could be a type mismatch in auth or hostname, or a bug in the SDK constructor. Swallowing all TypeErrors and retrying silently makes those failures very hard to diagnose. At minimum, you could check the exception message:

except TypeError as e:
    if "preview" not in str(e):
        raise
    return FoundryClient(auth=auth, hostname=self.host)

---

Issue: Client-level preview=True affects all SDK calls globally

Setting preview=True on the FoundryClient constructor (if the SDK supports it) will likely enable preview mode for every API call made through that client — not just build search. This is a broader change than the PR title implies. The schedule methods already accept an optional per-call preview argument. Enabling it globally could silently change the behavior of those calls.

It's worth confirming with the SDK docs whether client-level preview=True is scoped or global. If it's global, this PR changes more than intended.

---

Issue: Fallback doesn't fully protect older SDKs

Even when the client falls back to FoundryClient(...) (no preview), search_builds() and search_builds_paginated() still pass preview=True at the method level:

# orchestration.py
kwargs: Dict[str, Any] = {"preview": True}
...
response = self.service.Build.search(**kwargs)

On an older SDK that doesn't support preview, the client construction will succeed (via fallback) but the .search(preview=True) call will still raise an error. The two layers of preview (client-level and method-level) are tightly coupled but the fallback only handles one layer.

---

Minor: Duplicated stub classes in auth tests

In both test_get_client_prefers_preview_when_supported and test_get_client_falls_back_when_preview_unsupported, the stub classes (ConfidentialClientAuthStub, UserTokenAuthStub, FoundryClientStub) are redefined in each test method. These could be extracted to the class scope or a shared fixture to avoid duplication and keep the tests focused on their respective assertions.

---

Minor: Inconsistent mocking style

The new auth tests use monkeypatch.setitem(sys.modules, ...) while existing tests in the same file use with patch(...). Both work, but consistency is easier to read. Not a blocker.

---

Summary

Area	Assessment
Pagination bug fix	Correct and well-tested
preview=True at method level	Fine — matches SDK usage
try/except TypeError scope	Too broad — could mask real errors
Client-level global preview side-effect	Worth verifying scope with SDK docs
Fallback completeness on older SDKs	Fallback only partial; method-level calls still fail
Test duplication	Minor cleanup opportunity

The pagination fix is the most important change and it's correct. The preview-related fallback logic needs a bit more hardening before it's production-safe.

[claude]

Code Review (follow-up)

The previous two Claude review comments contain some inaccurate observations about this PR. Let me correct those and flag the genuine remaining concern.

---

Corrections to prior reviews

Review 1 made three claims that do not match the actual diff:

• Style inconsistency: The diff already uses {"preview": True} inline for search_builds(), not the empty-dict pattern described.
• Root cause left open: auth/oauth.py and auth/token.py are both updated in this PR to add client-level preview=True with fallback.
• Test gap for multi-page pagination: test_search_builds_paginated_collects_multiple_pages is present in the diff and covers two-page collection end-to-end.

Review 2 flagged the TypeError catch scope as too broad, but the code already guards this correctly in both layers with if "preview" not in str(e): raise. The tests test_get_client_reraises_unrelated_type_error and test_search_builds_reraises_unrelated_type_error verify that unrelated TypeErrors are re-raised.

---

What this PR gets right

• The pagination response shape fix is the core bug and is correctly fixed. ResponsePaginationHandler.collect_pages calls response.get("data", []), so the previous code was silently returning empty lists on every page. Remapping "builds" to "data" fixes this.
• The dual-layer preview approach (client constructor + call site, each with independent fallback) is solid defense-in-depth for SDK version compatibility.
• Test coverage is comprehensive: preview preferred, preview falls back, unrelated errors re-raised, single-page pagination, multi-page pagination, and call-level preview fallback.

---

One genuine concern: 2x SDK calls per page when call-level preview is unsupported

_search_with_optional_preview is stateless -- it always tries with preview=True first. If the installed SDK rejects the preview kwarg at call time (client-level falls back to no-preview, but then Build.search(preview=True) also fails), every page in a multi-page fetch makes two SDK calls: one that raises TypeError, one that succeeds. For N pages that is N wasted round-trips.

This is probably acceptable for a transient SDK compatibility window, but worth documenting on _search_with_optional_preview to set expectations, or caching the detection result on the instance after the first call so subsequent pages skip the try-and-fail branch.

---

Minor: no test for paginated preview fallback across multiple pages

test_search_builds_falls_back_when_preview_kwarg_unsupported tests search_builds() (single call), but there is no equivalent for search_builds_paginated(). A test where every page raises TypeError on the first attempt would confirm the fallback fires correctly on each page and results are still collected.

---

Summary

Area	Assessment
Core pagination shape bug fix	Correct
preview=True at both layers with fallback	Correct
TypeError scope guarded properly	Correct
Test coverage	Good, minor gap noted above
Performance: double SDK calls per page in fallback path	Worth a comment or cache

Solid, targeted fix. The double-call concern is minor and can be addressed with a comment or follow-up. Good to merge once the double-call behaviour is at least documented.