feat!: align types and client with latest API spec

[Kevin-layerV]

Summary

• Update all types to match current OpenAPI spec (breaking changes)
• Create endpoint moved from /v1/qurl to /v1/qurls
• create() parameter description renamed to label; added session_duration
• QURL dataclass: removed one_time_use, max_sessions, qurl_link, access_policy; added tags, custom_domain
• update(): removed access_policy param; added tags
• mint_link() expanded with full per-QURL settings
• New batch_create() method for POST /v1/qurls/batch
• AccessPolicy now includes ai_agent_policy
• Quota.rate_limits adds max_expiry_seconds; active_qurls_percent now nullable
• Quota.plan default sentinel changed from "" to "unknown" (affects any check like if not quota.plan or quota.plan == "")
• LangChain tools updated for label rename

Migration notes

No live consumers exist yet (the SDK is still pre-1.0 / pre-first-release), so this is a heads-up rather than an upgrade-on-fire migration guide. For the first consumers coming online after this lands:

create() — description → label, no absolute-expiry

# Before
client.create(target_url="https://example.com", description="Alice from Acme",
              expires_at=datetime(2026, 4, 1))

# After
client.create(target_url="https://example.com", label="Alice from Acme",
              expires_in="24h")

Per the OpenAPI spec (CreateQurlRequest at qurl/api/openapi.yaml:2300), the create endpoint only accepts relative expires_in, not absolute expires_at. To set an absolute expiry at creation time, use the minimum practical initial expiry on create and immediately update to the target absolute time:

# Minimum practical initial expiry ("1m") minimizes the window during
# which the QURL is live with the wrong expiry — the update below
# lands within a second or two on a healthy connection.
qurl = client.create(target_url="https://example.com", expires_in="1m")
client.update(qurl.resource_id, expires_at=datetime(2026, 4, 1))

Note: there's an unavoidable ~1-second window between create() and update() where the QURL is live with the initial 1-minute expiry. For most use cases this is negligible; if you need hard-synchronous absolute expiry guarantees, gate access behind your own authorization layer until after the update completes.

update() — access_policy removed (immutable after create)

Access policy is now set only at create time. The OpenAPI spec's UpdateQurlRequest schema accepts extend_by, expires_at, tags, and description but not access_policy — the field is intentionally immutable on an existing resource. If you were passing access_policy to update(), you have two migration paths:

1. Resource-level policy changes: Create a new resource via create() with the new policy and migrate callers away from the old resource. There's no in-place update.
2. Per-token policy variation: Use mint_link(resource_id, access_policy=...) to mint a new access token on an existing resource with a policy override scoped to that token — the resource's base policy stays the same, but the specific link you hand out carries the overridden policy.

# Before
client.update(resource_id, access_policy=AccessPolicy(ai_agent_policy=...))  # no longer valid

# After — option 1: new resource with new base policy
new_qurl = client.create(target_url="https://example.com",
                         access_policy=AccessPolicy(ai_agent_policy=...))

# After — option 2: per-token override via mint_link (base resource unchanged)
new_link = client.mint_link(resource_id,
                            access_policy=AccessPolicy(ai_agent_policy=...))

Quota.active_qurls_percent is now float | None

Previously defaulted to 0.0 when unlimited; now returns None. Callers doing arithmetic on this field will get a TypeError at runtime if they don't None-check:

# Before
usage_pct = quota.usage.active_qurls_percent  # always float

# After
usage_pct = quota.usage.active_qurls_percent or 0.0  # None when unlimited

Test plan

• ruff check passes clean
• mypy src/ passes with no errors (dedicated CI gate, not matrix-dependent)
• pytest tests/ -q all 121 tests pass
• New tests for batch create, date filters, expanded mint input, tags, label+session_duration
• New tests for POST retry safety (RETRYABLE_STATUS_POST = {429}), URL scheme validation, non-string target_url handling, batch shape guards (including bool-counts rejection)
• Async variants for all new methods

🤖 Generated with Claude Code

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured breaking change that cleanly aligns the SDK with the new API spec. The sync/async parity is maintained, tests are comprehensive, and the commit message properly signals the breaking change. A few items to consider:

Bugs / Issues

1. _parse_access_policy is dead code (src/layerv_qurl/_utils.py:105): The new helper is defined but never called anywhere. It was presumably intended to be used in parse_qurl() or another parser, but QURL no longer has an access_policy field, and MintOutput doesn't parse one either. Should be removed to avoid confusion, or wired up if it's still needed for a response type.

2. build_body doesn't recursively serialize nested dataclasses (src/layerv_qurl/_utils.py:94-99): When AccessPolicy is passed to mint_link() with a nested AIAgentPolicy, the serialization only goes one level deep. The ai_agent_policy field will be serialized as a raw AIAgentPolicy dataclass object rather than a dict. For example:

   policy = AccessPolicy(
       ip_allowlist=["10.0.0.0/8"],
       ai_agent_policy=AIAgentPolicy(block_all=True),
   )
   client.mint_link("r_abc", access_policy=policy)
   # body["access_policy"]["ai_agent_policy"] will be an AIAgentPolicy instance, not a dict

   This will likely cause a serialization error when httpx tries to JSON-encode the body. Consider adding recursive dataclass handling or using dataclasses.asdict() with a custom filter for None values.

3. _parse_access_policy falsy-check bug (src/layerv_qurl/_utils.py:108): if data.get("ai_agent_policy"): will skip parsing when block_all=False and the lists are empty (since {"block_all": False, ...} is truthy, this specific case is fine — but {} would be falsy). Minor edge case, but worth noting.

Design / API

4. batch_create accepts raw dicts (client.py:301, async_client.py:280): The parameter type is list[dict[str, Any]] which provides no type safety or IDE assistance. Consider accepting a typed dataclass (e.g., BatchCreateInput) or at minimum a TypedDict, matching the pattern used elsewhere in the SDK. This would also let you validate items client-side.

5. Date filter params use str instead of datetime: The list() and list_all() methods accept created_after, created_before, etc. as plain strings. The create() method already accepts datetime | str for expires_at — consider the same pattern here for consistency and better developer ergonomics.

Test Coverage

6. No test for AccessPolicy serialization in mint_link: The old test_access_policy_serialized was removed and replaced with test_mint_link_full_input, which passes an AccessPolicy with only ip_allowlist. Given the new ai_agent_policy nested field, a test with nested AIAgentPolicy would catch the serialization bug noted in point chore(main): release 0.2.0 #2.

7. No test for _parse_access_policy: Even if it's currently dead code, if it's intended for future use, it should have tests. If it's not needed, remove it.

8. No test for create() with custom_domain: The new custom_domain param on create() isn't exercised in any test.

Minor / Style

9. import builtins in TYPE_CHECKING block (client.py:35, async_client.py:38): This is used to disambiguate builtins.list[str] from the list() method. This works, but an alternative is from __future__ import annotations or using typing.List (though the latter is deprecated). The current approach is fine, just a bit unusual — worth a code comment if you keep it.

10. Good: Consistent formatting improvements across tests (one-arg-per-line for httpx.Response). The field(default_factory=list) usage for mutable defaults is correct. New types are properly exported in __init__.py.

Summary

The main actionable items are: remove or wire up the dead _parse_access_policy helper (#1), fix the nested dataclass serialization in build_body (#2), and add a test for AccessPolicy with AIAgentPolicy in mint_link (#6). The rest are suggestions for improved ergonomics.

🤖 Generated with Claude Code

[claude]

PR Review: feat!: align types and client with latest API spec

Overall Assessment

Clean, well-structured PR that aligns the SDK with the latest OpenAPI spec. The sync/async parity is maintained, tests are comprehensive, and the breaking changes are clearly documented. A few items worth addressing:

---

Issues

1. build_body silently drops False and 0 values (Bug)

_serialize_value correctly handles None, but build_body still does if v is None: continue — which is fine. However, the old build_body also didn't guard against falsy values, and now mint_link accepts one_time_use: bool. If a caller passes one_time_use=False, it will be included (since False is not None), which is correct. But max_sessions=0 would also be included — double-check that the API treats 0 the same as omitted, or consider whether you want to strip zero-valued ints.

2. batch_create accepts raw list[dict] — no validation (Design)

batch_create(items: list[dict[str, Any]]) passes user dicts directly to the API with zero client-side validation. Every other mutation method uses typed parameters + build_body. Consider at minimum:

• Documenting the expected dict shape more explicitly (beyond "at least target_url")
• Or adding a BatchCreateInput dataclass for type safety, consistent with the rest of the SDK's typed approach

3. _serialize_value doesn't handle list of dataclasses (Potential Bug)

_serialize_value handles None, datetime, and dataclass instances, but if someone passes a list containing dataclasses (e.g., a hypothetical future field), the list items won't be recursively serialized. Currently tags is list[str] so this isn't hit today, but the docstring says "recursively" which implies it should handle this. Consider:

if isinstance(v, list):
    return [_serialize_value(item) for item in v]

4. builtins.list type annotation pattern (client.py, async_client.py)

Using builtins.list[str] inside TYPE_CHECKING blocks to avoid shadowing with the list() method is correct but unusual. A more conventional approach is to use from __future__ import annotations (already using | union syntax so presumably ≥3.10), or rename the method parameter. Not blocking, just a style note — the current approach works.

5. Date filter params are str | None — no datetime support

list() and list_all() accept date filters as raw strings (created_after: str | None), while create() and update() accept datetime | str for expires_at. For API consistency within the SDK, consider accepting datetime | str | None for the date filter params too, and converting via .isoformat() like build_body does for other datetime fields.

---

Positive Highlights

• Recursive _serialize_value: Good refactor — the old build_body couldn't handle nested dataclasses (e.g., AIAgentPolicy inside AccessPolicy). The new approach is cleaner.
• Thorough test coverage: Batch create (success + partial failure), date filters, nested policy serialization, label/session_duration, custom_domain — all covered for both sync and async.
• AccessPolicy import removed from _utils.py: Since parse_qurl no longer constructs an AccessPolicy, the unused import was correctly cleaned up.
• Consistent formatting: The ruff-driven reformatting of multi-line dict literals and function calls improves readability.

---

Minor / Nits

• CreateOutput.qurl_id: str = "" — is empty string the right default, or should this be str | None = None for consistency with other optional fields? An empty string is truthy-ish in a way that None isn't, which could surprise callers doing if result.qurl_id: checks.
• The QURLStatus literal was narrowed from 5 values to 2 ("active", "revoked"). The | str escape hatch handles forward compat, but worth a note in migration docs that "expired", "consumed", "frozen" are no longer recognized literals for IDE autocomplete.
• parse_qurl defaults tags to [] via data.get("tags", []) — this creates a shared mutable default on each call which is fine, but note the QURL dataclass uses field(default_factory=list) correctly.

---

Summary

The PR is in good shape. The main actionable item is #3 (list serialization in _serialize_value) since it's a latent bug given the recursive serialization contract. Items #2 and #5 are design suggestions for a follow-up. Nice work on keeping sync/async parity and comprehensive test coverage.

[claude]

PR Review: feat!: align types and client with latest API spec

Overall Assessment

Well-structured breaking change that cleanly aligns the SDK with the updated API. The sync/async clients stay in lockstep, types are updated consistently, and test coverage is thorough (65 tests). A few items worth addressing:

---

Code Quality & Design

Positives:

• The new _serialize_value helper in _utils.py is a good refactor — recursive serialization handles nested dataclasses cleanly and eliminates the prior shallow-only limitation.
• build_list_params refactored to a dict-comprehension is more concise and maintainable.
• Batch types (BatchCreateOutput, BatchItemResult, BatchItemError) are well-designed with sensible defaults.
• Sync and async clients are kept perfectly in sync — no drift.

Suggestions:

• batch_create accepts list[dict[str, Any]] — consider accepting a typed dataclass (or at minimum documenting the expected dict shape beyond "at least target_url"). Raw dicts bypass the type safety the rest of the SDK provides. Even a TypedDict with target_url: str and optional fields would help IDE users.
• The import builtins inside TYPE_CHECKING to use builtins.list[str] (to avoid shadowing with the list method) works but is unusual. A common alternative is to use from __future__ import annotations which makes all annotations strings and avoids the shadowing entirely. Since the project already uses from __future__ import annotations, the builtins.list usage is unnecessary — you can just use list[str] directly since annotations are already strings and won't conflict with the method name.

---

Potential Bugs & Issues

1. _serialize_value with None short-circuit vs. build_body's None skip: build_body skips None at the top level, but _serialize_value returns None for None. This is fine for the current call sites but could be confusing if _serialize_value is used independently — a None nested inside a dataclass field would be serialized as None in the output (since the dict-comp filters is not None), but a None inside a list would be kept. Not a bug today, but worth a docstring note.

2. active_qurls_percent changed to float | None with default None: The parser uses .get("active_qurls_percent") without a default, so missing keys return None. This is correct, but any downstream code that previously assumed this was always a float (e.g., quota.usage.active_qurls_percent * 100) will now crash. This is a documented breaking change in the PR, so acceptable — just worth a changelog note.

3. CreateOutput.qurl_id defaults to "": Using empty string as default for an ID field could mask missing data. Consider None with str | None to make it explicit when the API doesn't return it, unless the API guarantees this field is always present (in which case the default is unnecessary).

---

Performance

• No concerns. The recursive _serialize_value is bounded by dataclass depth which is shallow. The batch endpoint caps at 100 items per the docstring. All good.

---

Security

• validate_id is still called for resource IDs before URL interpolation — good, prevents path traversal.
• batch_create sends raw dicts to the API. The API should validate, but if users pass user-controlled input directly into items, there's no client-side sanity check. This is fine for an SDK (validation is the server's job), but worth noting.

---

Test Coverage

• Comprehensive: batch create (sync + async), date filter params, expanded mint_link inputs, tags, label/session_duration on create, and the _serialize_value recursive helper are all covered.
• The AIAgentPolicy nested serialization test is a good edge case to include.
• One gap: no test for batch_create with mixed success/failure results (some items succeed, some fail in the same batch). The current test only covers all-success. Consider adding a test with succeeded=1, failed=1 to verify error parsing within a batch.

---

Minor Nits

• The list method docstring says status can be "active" or "revoked", but QURLStatus still has | str for forward compat. The docstring could mention this forward-compat behavior.
• Double space in _utils.py docstring: "receive a valid JSON body. Nested dataclasses" — two spaces after period.

---

Verdict

Approve with minor suggestions. The breaking changes are well-documented, code is clean, and test coverage is strong. The builtins.list usage and batch_create typing are the most actionable items.

[vikramlayerv]

Code Review

Good work on the API spec alignment. Test coverage is strong (65 tests, sync/async parity) and the recursive _serialize_value is a nice improvement. A few items to address:

---

Medium Severity

1. create() missing one_time_use, max_sessions, access_policy params (client.py, async_client.py)
The OpenAPI CreateQurlRequest schema includes these fields, and the TypeScript SDK supports them at creation time. Without them, Python users must create a QURL then separately mint_link() with policies — a different workflow. Please add to both sync and async clients.

2. batch_create() no client-side 1-100 validation (client.py:338, async_client.py:317)
The TypeScript SDK validates empty arrays and >100 items locally with descriptive errors. Python sends directly to the API, giving users a worse error experience. Add:

if not items:
    raise ValueError("items must not be empty")
if len(items) > 100:
    raise ValueError("batch_create supports at most 100 items")

3. batch_create() accepts raw list[dict] instead of typed items (client.py:338)
No type safety or autocompletion for batch item fields. Consider a typed BatchCreateItem dataclass (or accept both via union) to match the TypeScript SDK's typed interface.

4. Date filter params use str instead of datetime | str (client.py list/list_all)
create() accepts datetime | str for expires_at, but list() date filters (created_after, created_before, expires_before, expires_after) are plain strings. Should be consistent — accept datetime | str for all date params.

---

Low Severity (non-blocking)

5. CreateOutput.qurl_id defaults to "" (types.py:68) — str | None = None would be more explicit and prevent masking missing data.

6. QURLNetworkError/QURLTimeoutError don't extend QURLError — except QURLError won't catch network errors. The TypeScript SDK catches all errors under QURLError. Consider aligning the hierarchy so except QURLError is a reliable catch-all.

7. Cross-SDK inconsistency on expires_at — Python create() accepts it, TypeScript doesn't. Should align on whether this is supported.

8. PATCH retry inconsistency — Python retries 5xx on PATCH, TypeScript doesn't. Should agree on retry policy for mutating requests.

9. LangChain tools lack async _arun() — Despite having a full AsyncQURLClient, the tools only implement sync _run(). Async chains fall back to thread-pool execution.

10. builtins.list pattern — Unnecessary with from __future__ import annotations already in use.

---

Positive

• API key handling is solid (masked in repr, validated on construction)
• Path traversal protection via validate_id() regex
• Recursive _serialize_value handles nested dataclasses cleanly
• Error hierarchy maps well to RFC 7807
• LangChain integration handles optional dependency gracefully
• Test coverage is comprehensive with sync/async parity

[claude]

PR Review: feat!: align types and client with latest API spec

Overall Assessment

Clean, well-structured PR that aligns the SDK with the latest API spec. The changes are consistent across sync/async clients, types are properly updated, and test coverage is thorough. A few items worth addressing:

---

Code Quality & Best Practices

Strengths:

• Good use of _serialize_value to recursively handle nested dataclasses/datetimes — cleaner than the previous inline approach in build_body.
• Consistent parity between QURLClient and AsyncQURLClient — every new method/parameter appears in both.
• build_list_params refactored nicely with the dict-comprehension pattern.
• __init__.py exports are kept in sync with new types.
• LangChain tools updated to match the new API surface.

Suggestions:

1. batch_create accepts untyped list[dict[str, Any]] — Consider defining a BatchCreateInput dataclass (or at minimum a TypedDict) for the items parameter. This would give users IDE autocomplete and catch typos at the type-checking level rather than at API call time. The current raw-dict approach is fine for v1 but may be worth a follow-up.

2. builtins.list in type annotations (tags: builtins.list[str] | None) — This works but is unusual. It's needed because list is shadowed by the list() method name. An alternative is to use from __future__ import annotations (which is already imported) and rename the method parameter, or simply alias List from typing. Not a blocker since it's behind TYPE_CHECKING, but it may confuse contributors.

---

Potential Bugs or Issues

3. _serialize_value skips False and 0 correctly — Confirmed: build_body only checks if v is None: continue, so falsy values like False, 0, and "" are passed through to _serialize_value and preserved. Good.

4. parse_qurl hardcodes tags default to [] — data.get("tags", []) means every QURL object shares no default list instance (a new [] is created each call), so no mutable-default-sharing bug here. Correct.

5. AccessPolicy removed from _utils.py imports but AccessPolicy is still used — AccessPolicy was removed from the _utils.py import block. Double-check that parse_qurl no longer needs to parse access_policy from API responses. If the API ever returns access_policy on a QURL object, it will be silently dropped. This seems intentional per the spec change, but worth confirming.

---

Performance Considerations

6. _serialize_value recursion — The recursive serialization is fine for the expected depth (AccessPolicy → AIAgentPolicy is only 2 levels deep). No concerns here.

7. batch_create has no client-side validation of item count (1-100) — The docstring says "1-100 items" but there's no client-side guard. This is arguably fine (let the server validate), but a quick len check could give a better error message. Low priority.

---

Security Concerns

No security issues identified. The validate_id call is properly applied to all methods that take resource_id. No user input is interpolated unsafely.

---

Test Coverage

Strengths:

• 65 tests covering all new functionality: batch create (success + partial failure), date filters, expanded mint_link, tags, label/session_duration, custom_domain.
• Async variants for all new features.
• Nested AIAgentPolicy serialization test is a great edge case to cover.
• pytestmark skip for langchain tests when langchain-core is not installed — nice improvement.

Gaps:
8. No test for batch_create request body shape — The existing tests verify the response parsing, but unlike test_create_sends_correct_body, there's no test asserting the exact request body sent to /v1/qurls/batch. Consider adding one to verify {"items": [...]} is sent correctly.

9. No test for list_all with date filters — list_all was updated to pass through date filter params but there's no test verifying they propagate through the pagination loop. The sync test_list_all_paginates only tests status and page_size.

10. active_qurls_percent now nullable — No test for parse_quota when active_qurls_percent is absent/null in the response. The default changed from 0.0 to None, which could break downstream code that does arithmetic on this field.

---

Minor Nits

• The langchain.py blank line addition (between from langchain_core.tools import BaseTool and _HAS_LANGCHAIN = True) is a formatting-only change — fine, just noting it's a drive-by cleanup.

---

Summary

Solid PR. The breaking changes are well-documented in the PR description, types align cleanly, and test coverage is comprehensive. The main actionable items are:

• (8) Add a request-body assertion test for batch_create
• (9) Add a list_all test with date filters
• (10) Add a quota parsing test for nullable active_qurls_percent

Everything else is minor or cosmetic.

🤖 Generated with Claude Code

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured breaking change with good test coverage (65 tests). The recursive serialization fix, build_list_params simplification, and sync/async parity are all solid. A few items worth addressing:

---

Bug: _parse_access_policy ignores ai_agent_policy

src/layerv_qurl/_utils.py:114-123 — The new AIAgentPolicy type was added to AccessPolicy, and _serialize_value correctly handles it on the outbound path (serializing nested dataclasses). However, _parse_access_policy() never parses the ai_agent_policy field from API responses. Any ai_agent_policy returned inside an AccessToken.access_policy will be silently dropped.

def _parse_access_policy(data: dict[str, Any]) -> AccessPolicy:
    return AccessPolicy(
        ip_allowlist=data.get("ip_allowlist"),
        ip_denylist=data.get("ip_denylist"),
        geo_allowlist=data.get("geo_allowlist"),
        geo_denylist=data.get("geo_denylist"),
        user_agent_allow_regex=data.get("user_agent_allow_regex"),
        user_agent_deny_regex=data.get("user_agent_deny_regex"),
        # Missing: ai_agent_policy parsing
    )

Should add something like:

ai_policy = None
if data.get("ai_agent_policy"):
    ap = data["ai_agent_policy"]
    ai_policy = AIAgentPolicy(
        block_all=ap.get("block_all"),
        deny_categories=ap.get("deny_categories"),
        allow_categories=ap.get("allow_categories"),
    )

Inconsistent date/time parameter types

create() and mint_link() accept expires_at: datetime | str | None, but the new list filter parameters (created_after, created_before, expires_before, expires_after) only accept str | None. Users will reasonably expect to pass datetime objects to these filters too. Consider either:

• Accepting datetime | str | None and calling .isoformat() in build_list_params, or
• Documenting that these must be ISO strings

Minor: CreateOutput has both resource_id and qurl_id

CreateOutput now has resource_id (required) and qurl_id (defaults to ""). If these are the same value, one is redundant. If they're different, a docstring clarifying the distinction would help consumers. The default of "" for qurl_id also means callers can't distinguish "API didn't return it" from "API returned empty string" — None would be more idiomatic.

Consider: client-side validation for batch_create

The docstring says "1-100 items" but there's no client-side check. An empty list or >100 items would hit the API and return a less helpful error. A quick guard like:

if not items or len(items) > 100:
    raise ValueError("batch_create accepts 1-100 items")

would improve the developer experience. Not blocking — the API will enforce it — but it's a nice-to-have.

Nit: _serialize_value could handle Enum types

If any future field uses an Enum (or if QURLStatus becomes one), _serialize_value would pass it through as-is, which could cause JSON serialization issues. Not a problem today, just something to keep in mind.

---

What looks good

• Recursive serialization (_serialize_value) is a clean fix for the nested AIAgentPolicy bug and future-proofs against similar issues.
• build_list_params refactor — replacing 9 if/assign blocks with a dict comprehension is much cleaner.
• Sync/async parity — signatures, docstrings, and behavior match between both clients.
• Test coverage is thorough — batch create (success + partial failure), date filters, expanded mint with nested AccessPolicy/AIAgentPolicy, tags, label, custom_domain, and langchain skipif markers.
• LangChain tools properly updated for the label rename and corrected status values.

🤖 Generated with Claude Code

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured PR that cleanly aligns the SDK with the updated API spec. The sync/async parity is maintained, tests are thorough, and the breaking changes are clearly documented. Here is detailed feedback:

Code Quality and Best Practices

Strengths:

• Good extraction of _serialize_value() as a recursive helper — eliminates the previous limitation where nested dataclasses inside lists would not serialize correctly.
• build_list_params refactor to a declarative dict-comprehension is cleaner than the old if-chain.
• Consistent sync/async parity across all new methods (batch_create, expanded mint_link, date filters on list/list_all).
• Proper client-side validation on batch_create (empty list, >100 items).
• LangChain tools appropriately simplified to match the new API surface.
• Good addition of pytestmark = pytest.mark.skipif(not _HAS_LANGCHAIN, ...) in test_langchain.py.

Suggestions:

1. batch_create accepts list of dicts — consider typed input (client.py:332, async_client.py:309): The items parameter is untyped list[dict[str, Any]]. For a public SDK method, consider accepting a list of dataclasses (e.g., BatchCreateItem) or at minimum a TypedDict so users get IDE autocomplete and type checking. The raw dict approach means typos in keys like "taret_url" will not be caught until runtime. This is a minor nit given it matches the REST wire format, but worth considering for a future iteration.

2. _parse_access_policy has a lazy import (_utils.py:82): from layerv_qurl.types import AIAgentPolicy is imported inside the function body. Since AIAgentPolicy is already available at module scope (the module imports from types at the top), this import is unnecessary — AIAgentPolicy could simply be added to the existing import block at the top of _utils.py.

3. CreateOutput.qurl_id defaults to empty string rather than None (types.py:87): Other optional string fields in the codebase use None as the default. An empty string default means callers cannot distinguish "server did not return this field" from "server returned an empty string". Consider qurl_id: str | None = None for consistency, and update parse_create_output to use .get("qurl_id") instead of .get("qurl_id", "").

Potential Bugs / Issues

4. _serialize_value does not handle dict values (_utils.py:79-92): If a user passes a plain dict (not a dataclass) as a nested value, it passes through as-is without recursing into it. This means datetimes nested inside plain dicts will not be serialized to ISO strings. This is not a regression (the old code did not handle it either), but now that you have a recursive serializer, it might be worth handling dict for completeness.

5. batch_create items are not serialized through build_body/_serialize_value (client.py:348, async_client.py:326): The items list is passed directly as {"items": items}. If a user includes datetime objects or dataclasses in their item dicts, they will not be serialized correctly. Consider running each item through build_body() or _serialize_value().

Performance Considerations

• No concerns. The recursive serialization adds negligible overhead. Batch endpoint is capped at 100 items client-side, which is appropriate.

Security Concerns

• No security issues identified. Input validation (validate_id, batch size limits) is appropriate. No user-controlled strings are interpolated unsafely.

Test Coverage

Strengths:

• Excellent coverage of new features: batch create (success + partial failure), date filters, expanded mint_link, tags in update, label + session_duration in create, custom_domain, nested AIAgentPolicy serialization.
• Both sync and async variants tested for all new paths.
• 65 tests passing is solid.

Gaps to consider:

• No test for batch_create with an empty list or >100 items (the ValueError paths). These are easy to add and would cover the validation logic.
• No test for list_all with date filter params being propagated through pagination (i.e., verifying the second page request also includes the date filters).
• The AccessPolicy serialization test was replaced rather than extended — there is no longer a test that verifies create() correctly serializes an AccessPolicy in the request body. The new test only covers mint_link().

Breaking Change Documentation

The PR description clearly lists all breaking changes, which is great. Since this will go through Release Please, make sure the commit message retains the ! in feat!: so it triggers a major version bump.

---

Verdict: Looks good with minor suggestions. The core changes are solid and well-tested. Items 2, 3, and 5 above are the most actionable. Nice work keeping sync/async in lockstep.

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured breaking change that cleanly aligns the SDK with the updated API. The sync/async clients stay in lockstep, tests are thorough, and the new types are well-modeled. A few observations below.

Code Quality

• _serialize_value is a nice refactor. Extracting the recursive serializer from build_body keeps things DRY and handles nested dataclasses + lists correctly. Good improvement over the inline approach.
• build_list_params rewrite — the dict-comprehension approach is cleaner than the old if-chain. Reads well.
• Sync/async parity — both clients were updated identically which is important for this project. No drift detected.
• import builtins under TYPE_CHECKING — the builtins.list[str] annotation for tags and builtins.list[dict[str, Any]] for batch_create is a valid workaround for the list shadowing from the list() method. This is fine, though a type alias (e.g., _List = list at module level) would be slightly more readable if this pattern grows.

Potential Issues

1. _parse_access_policy has a local import of AIAgentPolicy (line 116 of _utils.py) even though all other types from layerv_qurl.types are imported at the top of the file. This is likely a leftover from an earlier iteration. Consider moving it to the top-level import block for consistency — there's no circular dependency risk since _utils.py already imports from types.py.

2. CreateOutput.qurl_id defaults to "" while resource_id is required — both seem to represent IDs from the API response. Is there a case where the API returns resource_id but not qurl_id? If not, qurl_id could be required too. If qurl_id is truly optional in the response, None would be a more accurate default than "" to distinguish "missing" from "empty string".

3. _serialize_value returns None early but build_body already skips None values. The None check in _serialize_value only matters for nested fields (e.g., a dataclass field that is None). This is correct behavior, just noting it's intentional — nested None fields get dropped by the dataclass serialization branch's if getattr(v, f.name) is not None guard, so the top-level None return is only reachable via list items. Consider whether a None inside a list should be preserved or dropped — currently it's preserved as None in the JSON array, which may or may not match API expectations.

4. batch_create accepts list[dict[str, Any]] — this is flexible but untyped. Users won't get IDE autocomplete for required fields like target_url. A BatchCreateItem dataclass (or TypedDict) would improve DX. Not blocking, but worth considering for a follow-up.

Security

• validate_id is correctly applied before path interpolation in update, mint_link, get, revoke, and delete. No SSRF/path-traversal issues.
• The batch endpoint sends user-provided dicts directly as the body. This is fine since the API server is the trust boundary, but the 100-item limit validation is a good client-side guard.

Performance

• No concerns. The recursive serializer adds negligible overhead for the small payloads involved.

Test Coverage

• Strong coverage: 65 tests passing, with new tests for batch create, date filters, expanded mint input, tags, and label+session_duration.
• Async variants are covered.
• One suggestion: consider adding a test for _serialize_value with deeply nested structures (e.g., an AccessPolicy with AIAgentPolicy inside a build_body call) to ensure the recursive serialization works end-to-end. The current tests exercise it implicitly through client methods, but a direct unit test would guard against regressions.

Minor Nits

• The status docstring on list() was narrowed to just "active", "revoked" — but QURLStatus still has | str for forward compat. The docstring is fine as guidance, just noting the type still accepts arbitrary strings which is the right call.

Verdict

Looks good. The breaking changes are well-documented in the PR description, the implementation is consistent across sync/async, and test coverage is solid. The items above are suggestions, not blockers.

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured PR that cleanly aligns the SDK with the updated API spec. The sync/async parity is maintained, tests are comprehensive, and the breaking changes are clearly documented. A few observations:

Positives

• Excellent test coverage — 65 tests covering batch create (success, partial failure, validation), date filters, expanded mint_link params, tags, nested AIAgentPolicy serialization, and async variants for all new methods.
• Clean recursive serialization — _serialize_value() in _utils.py is a good refactor that handles nested dataclasses and datetime serialization uniformly, replacing the previous flat approach.
• Good input validation on batch_create() (empty list, >100 items) with clear error messages.
• Sync/async parity — every new feature has both sync and async implementations with matching tests.

Issues & Suggestions

1. builtins.list type annotation pattern (minor)
In client.py and async_client.py, builtins.list is imported under TYPE_CHECKING and used for tags and batch_create params. This works but is non-standard. The usual pattern is from __future__ import annotations (which both files already have), which makes list[str] valid in annotations without needing builtins.list. The builtins import appears to exist to avoid shadowing with the list() method name — but since list is only used in type annotations (not at runtime), from __future__ import annotations already handles this. Consider using plain list[str] and list[dict[str, Any]] instead.

2. _serialize_value returns None for None but build_body already skips None (nitpick)
_serialize_value has an explicit if v is None: return None check, but build_body already does if v is None: continue. The None check in _serialize_value is still necessary for nested dataclass fields (where None fields are filtered), so this is fine — just noting the dual filtering is intentional.

3. batch_create accepts list[dict[str, Any]] — consider typed input (suggestion)
The items parameter is untyped dicts. For a public SDK, a typed input (e.g., a BatchCreateItem dataclass or at least a TypedDict) would give users better IDE autocomplete and catch errors earlier. This could be a follow-up.

4. QURL dataclass still has description but create() renamed it to label (potential confusion)
QURL.description is kept (presumably the API still returns it on GET), but create() now uses label and update() still accepts description. The PR summary says "removed description" but it's still present on QURL and in update(). This seems correct for the API, but worth double-checking the spec to confirm description and label are truly distinct fields.

5. CreateOutput has both resource_id and qurl_id — document the distinction
The new qurl_id field on CreateOutput is optional. It would help users to have a brief docstring or comment explaining when qurl_id vs resource_id is used.

6. active_qurls_percent default change (minor breaking)
Usage.active_qurls_percent changed from float = 0.0 to float | None = None. This is a subtle breaking change for consumers who do arithmetic on this field without None-checking. Worth mentioning in migration notes if there are any.

7. QURLStatus literal narrowed from 5 values to 2 (breaking)
QURLStatus dropped "expired", "consumed", "frozen". The | str union keeps it forward-compatible, but downstream code matching on the old literals will lose type-checker coverage. This is fine given the API spec change, just ensure it's called out in changelog.

Security

• No concerns. API keys are still masked in logs, input validation is present on batch sizes, and no user input is interpolated unsafely.

Performance

• _serialize_value is recursive but the nesting depth is bounded (AccessPolicy > AIAgentPolicy is max 2 levels). No concerns.

Verdict

Looks good to merge. The breaking changes are well-justified by the API spec alignment. Consider the builtins.list cleanup and typed batch input as follow-ups.

🤖 Generated with Claude Code

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured breaking change that cleanly aligns the SDK with the latest API spec. The sync/async parity is maintained, tests are thorough, and the new _serialize_value helper is a solid improvement. A few items to consider:

Issues & Suggestions

1. _serialize_value strips None from dicts but not from lists (minor inconsistency)

In _utils.py, _serialize_value filters out None values from dicts (if val is not None) and omits None dataclass fields, but lists pass through None items unchanged. This is likely fine for current usage, but worth noting as a potential surprise if someone passes tags=["a", None, "b"].

2. batch_create accepts raw dicts — consider typed input

batch_create(items: list[dict[str, Any]]) is flexible but loses all type safety. Users won't get IDE autocomplete or type-checking for the item fields. Consider whether a BatchCreateItem dataclass (or at minimum a TypedDict) would be worthwhile for a future follow-up, since this is a public API surface.

3. active_qurls_percent nullable change needs a migration note

Usage.active_qurls_percent changed from float = 0.0 to float | None = None. The inline comment says "None-check before arithmetic" which is good, but any existing callers doing quota.usage.active_qurls_percent * 100 will now get a runtime TypeError instead of a wrong-but-safe 0.0. Since this is a breaking change PR, this is acceptable — just make sure it's called out in release notes.

4. QURL dataclass: description field kept but label not added

create() renamed description → label, and CreateOutput has a new label field, but the QURL dataclass still has description (no label). Meanwhile update() still accepts description. This asymmetry seems intentional (the API likely kept description on the resource while label is for the access token), but it could confuse users. A brief docstring clarification on QURL.description vs CreateOutput.label would help.

5. build_list_params type annotation could be tighter

The intermediate pairs dict is typed dict[str, str | int | datetime | None] but limit is int | None while the rest are str | None or datetime | str | None. This works, but the output comprehension silently calls str(v) on everything, which means a datetime passed as created_after will use str() (Python's default repr) instead of .isoformat() only if it's not a datetime instance. Actually, looking more closely — you do handle datetime with .isoformat() in the comprehension, so this is correct. Good.

6. Missing __all__ export for field

Not a bug, but field is imported from dataclasses in types.py — it's not in __all__ which is fine since it's an implementation detail, just confirming this was intentional.

Positive Highlights

• Recursive _serialize_value — Clean refactor from inline serialization to a reusable recursive helper. Correctly handles nested dataclasses, datetimes, lists, and dicts.
• build_list_params rewrite — Much cleaner than the previous if-chain approach.
• Test coverage is excellent — 65 tests covering batch create (success, partial failure, validation), date filters, expanded mint_link, tags, label/session_duration, nested AIAgentPolicy serialization, and async variants for all new features.
• pytestmark skip for langchain tests — Good defensive addition for environments without langchain-core.
• Consistent sync/async parity — Every new method in client.py has its mirror in async_client.py with matching signatures and tests.

Security

• No concerns. The validate_id() call is maintained on all methods that interpolate user input into URL paths. The batch_create limit of 100 items is a reasonable client-side guard.

Verdict

Approve — this is ready to merge. The items above are minor and can be addressed in follow-ups if desired.

[claude]

Code Review — PR #8: feat!: align types and client with latest API spec

Overall

Well-structured breaking change PR. The sync/async client parity is maintained, the _serialize_value extraction is clean, _utils.py centralizes all parsing logic, and the test coverage increase from ~50 to ~80 test cases is substantial. The commit history tells a clear story of iterative review feedback.

CI passes across Python 3.10, 3.12, and 3.13. ✓

---

Positive highlights

• _serialize_value recursion (_utils.py:83-99): Clean recursive serializer that handles dataclasses, datetimes, lists, and dicts uniformly. The None-stripping for dataclass fields and dict values is consistent with build_body's top-level None-dropping.
• build_list_params simplification (_utils.py:334-361): Replacing 9 repetitive if/assign blocks with a dict comprehension is a nice readability win.
• Client-side batch validation (client.py:359-362): Catching empty list and >100 items locally gives descriptive errors instead of cryptic API 400s.
• builtins.list for class method annotations (client.py:283, async_client.py:272): The commit message explains the mypy scoping issue well — this is a real Python 3.10-3.12 gotcha.
• pytestmark = pytest.mark.skipif for langchain tests (test_langchain.py:28): Prevents false failures in environments without langchain-core.

---

Issues & suggestions

1. No test coverage for parsing AIAgentPolicy from API responses (medium)

_parse_access_policy now correctly parses ai_agent_policy from response data (_utils.py:119-125), but there's no test exercising this inbound parsing path. The existing tests only verify outbound serialization (e.g., test_mint_link_nested_ai_agent_policy, test_create_with_access_policy).

Consider adding a test for get() that returns a QURL with an AccessToken containing a nested ai_agent_policy, then asserting token.access_policy.ai_agent_policy.block_all etc. are correctly parsed.

2. No test for datetime objects in list date filter params (low)

build_list_params (_utils.py:357-358) calls .isoformat() on datetime values, but all tests pass string timestamps to list(). Consider adding one test passing a datetime object to e.g. created_after to verify the isinstance(v, datetime) branch.

3. _serialize_value None handling: lists preserve None elements, dicts drop them (low, note for awareness)

In _serialize_value:

• Dicts (line 98): if val is not None — None values are stripped.
• Lists (line 96): [_serialize_value(item) for item in v] — None items pass through (returning None from line 87).

This means [None, "a"] serializes as [None, "a"], but {"a": None, "b": 1} serializes as {"b": 1}. This is correct for the current API (body fields should drop nulls, but list order matters), but worth documenting if the function is expected to be reused.

4. _serialize_value could handle Enum types (nit, future-proofing)

If any API field ever uses a Python Enum, _serialize_value would pass it through as-is, which would fail JSON serialization. Not a current issue, just noting.

5. Consider testing list_all() with date filters (low)

Tests verify list() passes date filter params, but there's no explicit test that list_all() propagates them through the pagination loop. The implementation looks correct — it delegates to list() — but a quick test would catch regressions if someone refactors the pagination.

---

Breaking changes checklist

All correctly identified in the commit message:

Change	Impact
create() endpoint /v1/qurl → /v1/qurls	All existing create() calls work unchanged (URL is internal)
create(description=) → create(label=)	Callers using named param must update
update() removes access_policy param, adds tags	Callers passing access_policy to update() will get TypeError
active_qurls_percent: float → float | None	Callers doing arithmetic without None-check will get TypeError
QURLStatus literals narrowed to active | revoked	No runtime impact (| str catches all) — only IDE autocomplete changes
QURL dataclass: fields removed/added	Callers accessing removed fields (one_time_use, max_sessions, qurl_link, access_policy) will get AttributeError

The active_qurls_percent breaking change has a helpful inline comment (types.py:151). Consider also noting it in the changelog or migration guide if one exists.

---

Summary

This is a solid PR. The code is clean, well-tested, and the sync/async implementations are properly mirrored. The main gap is test coverage for the inbound AIAgentPolicy parsing path (#1 above). The rest are minor suggestions. Good to merge once #1 is addressed or accepted as a follow-up.

[claude]

Code Review — PR #8: feat!: align types and client with latest API spec

Overall this is a well-structured breaking change that methodically aligns the Python SDK with the current API spec. The sync/async clients are kept in lock-step, the recursive serializer is a real improvement, and test coverage is thorough (65 tests including async mirrors). A few items worth addressing before merge:

---

Issues

1. _serialize_value silently drops None values in dicts (potential data loss)
_utils.py:98 — The dict branch filters out None values:

if isinstance(v, dict):
    return {k: _serialize_value(val) for k, val in v.items() if val is not None}

This is appropriate for build_body() (top-level request body), but _serialize_value is also used recursively inside lists and nested dicts. If the API ever expects an explicit null in a nested position (e.g. "access_policy": {"ai_agent_policy": null} to clear a policy), this will silently drop it. The dataclass branch already skips None fields, so the dict branch doing it too creates a double filter. Consider whether the None-stripping in the dict case should be limited to the top-level build_body call only.

2. _serialize_value returns None for None input, but build_body already skips None
_utils.py:85-86 — The if v is None: return None branch at the top of _serialize_value is dead code in the build_body path (which skips None before calling _serialize_value). It is reachable from the list recursion path though (list items that are None). Not a bug, but the intent is ambiguous — a None inside a list would be preserved, while a None inside a dict would be dropped. Worth a clarifying comment.

3. batch_create items type is list[dict[str, Any]] — no type safety for item shape
client.py:348 / async_client.py:337 — The batch items are plain dicts. Users get no IDE autocompletion or type checking on what keys are valid. Consider defining a BatchCreateItem dataclass (or TypedDict) to match create()'s parameters. This would also let you reuse the same build_body serialization instead of relying on users passing raw dicts with correct key names. (Not a blocker, but worth considering for a v2 SDK.)

4. CreateOutput.qurl_id default is None but one test mock returns ""
types.py:92 — qurl_id: str | None = None. But in test_create_omits_none_values, the mock response sets "qurl_id": "". The parse_create_output will faithfully set qurl_id = "" — so callers can't reliably use if result.qurl_id: to check presence (empty string is falsy). If "" is a valid API response for "no qurl_id", then parse_create_output should normalize it to None:

qurl_id=data.get("qurl_id") or None,

Or if "" is not a real API response, fix the test mock.

5. active_qurls_percent breaking change could be smoother
types.py:151 — Changing from float = 0.0 to float | None = None is a breaking change that will cause TypeError for existing callers doing arithmetic on this field. The inline comment is good, but this should be called out in the CHANGELOG / release notes for Release Please. Make sure the BREAKING CHANGE: footer in the commit message explicitly mentions this field.

---

Minor / Nit

6. build_list_params type annotation is slightly imprecise
_utils.py:348 — pairs is typed as dict[str, str | int | datetime | None] but it can also hold QURLStatus (a Literal | str union). Not wrong since str covers it, just slightly misleading when reading the annotation.

7. No async test for batch_create validation (empty / >100)
Sync versions have test_batch_create_empty_raises and test_batch_create_over_100_raises, but there are no async counterparts. Since the validation is synchronous (ValueError raised before any await), this is low risk — but for completeness and symmetry with the rest of the test suite, consider adding them.

8. _HAS_LANGCHAIN is imported from langchain.py in tests
test_langchain.py:30 — from layerv_qurl.langchain import _HAS_LANGCHAIN imports a private symbol. This works but is fragile if the module internals change. A minor coupling concern.

9. test_create_sends_correct_body no longer tests one_time_use/max_sessions
The old test verified that one_time_use=True was sent in the body. The new test only sends label="test". Since create() now accepts one_time_use, max_sessions, session_duration, and access_policy, it'd be good to have at least one test that verifies these are all serialized correctly in create() specifically (beyond the access_policy test that was added).

---

Positives

• The _serialize_value recursive helper is a clean refactor that properly handles nested dataclasses, lists, and dicts in one place.
• Sync/async parity is maintained rigorously across all new methods.
• Client-side validation on batch_create (1-100 items) gives clear errors before hitting the API.
• The builtins.list workaround for mypy class-scope resolution is correctly documented with a commit explaining why.
• The build_list_params refactor from 9 if/assign blocks to a dict comprehension is much cleaner.
• Good test coverage overall — new tests for batch create (success, partial failure, datetime serialization), date filters, expanded mint_link, tags, nested AIAgentPolicy, and custom_domain.

---

Verdict: Looks good overall. Items 1 and 4 are the ones I'd most want addressed before merge; the rest are nice-to-haves or can be follow-ups.

[claude]

Code Review: feat!: align types and client with latest API spec

Overall Assessment

This is a substantial, well-structured PR that aligns the Python SDK with the current OpenAPI spec. The changes are thorough — types, endpoints, validation, error handling, and tests have all been updated in lockstep. The sync and async clients are kept in near-perfect parity, and the test count nearly doubled (65 → 101). The iterative commit history shows multiple review rounds already addressed, which is reflected in the code quality.

---

Strengths

• Recursive serialization (_serialize_value): The approach of separating top-level None-stripping (in build_body) from nested preservation is well-reasoned. The docstring clearly explains the asymmetry between dict and dataclass None handling, which will save future readers time.
• RFC 7807 error handling: The detail fallback chain (detail → message → title → HTTP {status}) with legacy envelope support is robust. The QURLError constructor also defaults detail to title so Exception.args is never misleadingly empty.
• Spec-derived validation: Client-side pre-flight checks mirror API constraints, providing instant feedback. The per-index attribution in batch_create (items[N]: ...) is a nice touch for debugging.
• allow_statuses mechanism: The approach to whitelisting HTTP 400 on batch_create is clean — it lets the structured per-item error body through without disrupting the error path for 401/403/429/5xx.
• Test coverage: Good mix of happy path, boundary (max_sessions 0 and 1000), validation rejection, error fallback, and async symmetry tests.

---

Issues

1. TokenStatus and QuotaPlan not exported in __init__.py

QURLStatus is exported, but the two new type aliases — TokenStatus (used by AccessToken.status) and QuotaPlan (used by Quota.plan) — are not in __all__ or imported in __init__.py. Users who want to type-annotate against these need to reach into layerv_qurl.types directly, which is inconsistent.

Suggestion: Add both to the imports and __all__ in __init__.py.

2. batch_create HTTP 400 passthrough assumes structured body

allow_statuses=(400,) treats any 400 response as a valid BatchCreateOutput. If the API returns a 400 for a non-batch reason (e.g., malformed JSON, rate limiting at a proxy layer, or an API regression), parse_batch_create_output would silently produce a BatchCreateOutput(succeeded=0, failed=0, results=[]) due to .get() defaults rather than raising an error.

Consider a defensive check after parsing — e.g., if the response has an "error" key instead of the expected batch structure, fall back to raising a QURLError. Something like:

envelope = response.json()
if "error" in envelope and "data" not in envelope:
    raise parse_error(response)

This would make the 400 passthrough more surgical.

3. _require_valid_tags runtime type check could be tightened

At _utils.py:170, if a non-string value sneaks into the tags list (e.g., tags=[123]), the len(tag) call still succeeds on an int in Python... wait, no — len(123) raises TypeError. But the isinstance(tag, str) check covers this: not isinstance(tag, str) is True for 123, then type(tag).__name__ is used. This actually works correctly. Ignore this point.

4. Minor: type parameter shadows builtin in QURLError.__init__

errors.py:36 — the type parameter in QURLError.__init__ shadows the built-in type() function. This is a common pattern in RFC 7807 implementations and causes no runtime issues (it's method-scoped), but worth noting for anyone who might try to use type(...) inside the constructor body later.

5. update() allows tags=[] to pass the "at least one field" check

Empty list tags=[] passes the tags is None check (it's not None), so update("r_abc", tags=[]) goes through. This is intentional per the test test_update_accepts_empty_tags_to_clear, and the docstring documents it. Good — just calling it out as a deliberate design choice.

---

Nits (non-blocking)

1. _utils.py:498 — dict[str, int | str | datetime | None]: The type for pairs includes int for limit, but the comprehension converts everything to str via str(v). This works correctly but the annotation is slightly wider than needed since int is always stringified.

2. types.py:180 — plan: QuotaPlan = "": An empty string as the default for plan means Quota objects from responses missing plan will have plan == "" rather than None. This is consistent with the existing pattern but could surprise callers checking if quota.plan:. A str | None = None default would be more Pythonic for "unknown."

3. Docstring consistency: create() documents that expires_in is the only temporal parameter accepted, but create() in the existing codebase (before this PR) accepted expires_at. The breaking change of removing expires_at is noted in the commit messages but not in a migration guide. Since this is a feat! PR, consumers should check the release notes — worth ensuring Release Please captures the expires_at removal prominently.

4. _serialize_value is called for every field in build_body, including simple strings and ints that don't need serialization. For hot paths this could matter, but for an HTTP client making network calls, the overhead is negligible.

---

Security

• validate_id with the regex ^[a-zA-Z0-9_\-]+$ prevents path traversal — good.
• require_resource_id_prefix adds defense-in-depth for delete operations.
• No secrets or credentials in test fixtures (using lv_live_test placeholder).
• API key masking in __repr__ is correct.

Breaking Changes Checklist

The PR correctly identifies these breaking changes:

• create(): expires_at param removed, description renamed to label
• QURL dataclass: fields removed/added
• active_qurls_percent: float → float | None
• Endpoint path: /v1/qurl → /v1/qurls

Recommendation: Consider documenting the breaking changes in a migration section in the release notes, since downstream users will need to update their code.

---

Verdict

Approve with minor suggestions. The code is well-written, thoroughly tested, and the architectural decisions are sound. The main actionable items are:

1. Export TokenStatus and QuotaPlan from __init__.py for public API consistency.
2. Consider hardening the batch_create 400 passthrough against non-batch error responses.

Nice work on a clean, comprehensive API alignment. 🏗️

[justin-layerv]

Port of all qurl-typescript #14 seam fixes + mypy gate hardening

Brings the Python SDK into full parity with the improvements landed on qurl-typescript and qurl-mcp during the recent review/seam-audit rounds, cross-referenced against qurl/api/openapi.yaml and the Go handler code. Two commits:

• 3b32bea — code changes across types.py, errors.py, _utils.py, client.py, async_client.py
• 39891e0 — CI hardening so mypy errors always fail the build

🔴 Critical — real bug

parse_error detail fallback. RFC 7807 leaves detail optional and the qurl Error schema only requires type/title/status/code. The parser used err.get("detail", ""), producing "Forbidden (403): " when the API omitted detail. Now falls back detail → message → title → HTTP {status}. QURLError also defaults detail to title in its constructor so Exception.args is never empty-string padded.

RFC 7807 structured fields

• QURLError now carries type and instance (problem-type URI and occurrence URI). Both were silently dropped before.
• parse_error extracts them from the envelope.

Backward compatibility

• Legacy {error: {code, message}} envelope supported in the fallback chain. Matches qurl-typescript's strategy for handling pre-RFC-7807 error shapes.

Type narrowing

• QURLStatus clarified as resource-only ("active" | "revoked" | str).
• New TokenStatus for AccessToken ("active" | "consumed" | "expired" | "revoked" | str) — per QurlSummary.status in the spec, tokens have a wider enum than resources.
• New QuotaPlan ("free" | "growth" | "enterprise" | str); Quota.plan uses it. (Literal | str) pattern so the API can add new plans without a breaking SDK change.

Spec-derived input validation

New validate_create_input / validate_update_input / validate_mint_input helpers in _utils.py enforcing every constraint documented on the request schemas in openapi.yaml:

Field	Spec constraint	Now enforced in
target_url	maxLength: 2048	create(), batch_create items
label (create + mint)	maxLength: 500	create(), mint_link(), batch items
description (update)	maxLength: 500	update()
custom_domain	maxLength: 253	create(), batch items
max_sessions (create + mint)	minimum: 0, maximum: 1000	create(), mint_link(), batch items
tags (update)	maxItems: 10, each 1-50 chars, regex ^[a-zA-Z0-9][a-zA-Z0-9 _-]*$	update()

batch_create runs validate_create_input on each item and attributes errors by index (items[N]: ...) so bulk mistakes fail fast before the API round-trip.

Mutual-exclusion pre-flight checks

• update: rejects both extend_by + expires_at
• update: rejects empty input (at least one field required)
• mint_link: rejects both expires_in + expires_at
• extend() inherits the update() checks via delegation.

delete() r_ prefix enforcement

Per the OpenAPI spec DELETE /v1/qurls/:id description (openapi.yaml:422): "Requires a resource ID (r_ prefix). To revoke a single token, use DELETE /v1/resources/:id/qurls/:qurl_id". New require_resource_id_prefix helper raises ValueError client-side for q_ IDs with a clear message pointing at the token-scoped endpoint.

batch_create HTTP 400 passthrough

The API returns a populated BatchCreateOutput body on HTTP 400 when every item fails validation — confirmed at qurl/internal/api/handlers/server.go:1126. Added allow_statuses to _raw_request and _request; batch_create whitelists 400 so per-item errors are surfaced instead of being swallowed by the generic raise-on-error path. Non-400 errors (401/403/429/5xx) still raise the appropriate QURLError subclass. Matches qurl-typescript and qurl-mcp.

create() parameter cleanup (breaking)

Dropped the spurious expires_at kwarg from both sync and async create(). CreateQurlRequest in openapi.yaml:2299 has only expires_in — the previous signature let callers pass a field the API silently ignored.

Dual-prefix documentation

get/update/extend/mint_link docstrings now document that both r_ (resource) and q_ (QURL display) IDs are accepted; the API resolves q_ IDs to the parent resource automatically. delete() stays narrow (r_ only) matching its client-side enforcement.

parse_create_output: normalize empty qurl_id to None

Empty-string qurl_id is normalized to None so callers can use if result.qurl_id: as a presence check. Addresses reviewer issue #4.

_serialize_value: stop stripping None from nested dicts

Previously the dict branch filtered out None values, which would silently drop explicit nulls callers send to clear nested fields (e.g. {"access_policy": {"ai_agent_policy": null}}). Top-level None-stripping still happens in build_body since that serves the "drop unset kwargs" case. Nested None is now preserved. Addresses reviewer issues #1 and #2.

Misc from review

• build_list_params type annotation tightened (reviewer nit build(deps): Bump anthropics/claude-code-action from 1.0.69 to 1.0.73 #6).
• test_update_with_tags corrected to use spec-compliant tags — the previous test used team:engineering with a colon the ^[a-zA-Z0-9][a-zA-Z0-9 _-]*$ regex rejects. The server would reject those tags too.
• test_create_sends_correct_body now covers one_time_use, max_sessions, and session_duration alongside label (reviewer nit build(deps): Bump anthropics/claude-code-action from 1.0.69 to 1.0.83 #9 gap).
• Async variants for batch_create empty/>100 validation (reviewer nit build(deps): Bump anthropics/claude-code-action from 1.0.69 to 1.0.77 #7 symmetry gap).

mypy gate hardening (commit 39891e0)

Two related CI fixes to ensure mypy errors always fail the build:

1. Moved mypy src/ into a dedicated type-check job. The previous setup ran mypy as a step inside the test job guarded by matrix.python-version == '3.12' — if the matrix is ever restructured and 3.12 dropped, the mypy gate would silently vanish. The new job pins Python 3.10 (matching python_version in pyproject.toml, which is the strictness contract the SDK promises consumers) and always runs. The notify job now depends on both test and type-check, so a mypy failure fails the overall build.

   Also added fail-fast: false to the test matrix so a single failing Python version doesn't cancel sibling legs.

2. Added langchain-core to the [dev] extra. The optional langchain integration was only installed via [langchain], so developers running pip install -e ".[dev]" locally (without the langchain extra) saw spurious mypy errors about missing modules and unused type-ignore comments in src/layerv_qurl/langchain.py. CI already installed [dev,langchain], but local envs drifted. Pulling langchain-core into [dev] aligns local dev with CI.

Tests

74 → 108 passing (+27 new seam/validation tests, +7 previously-skipped langchain tests now running since langchain-core is installed).

Coverage:

• Create rejection: target_url > 2048, label > 500, custom_domain > 253, max_sessions > 1000 / negative
• Create boundaries: max_sessions at 0 and 1000 both accepted
• Update rejection: description > 500, > 10 tags, tag > 50 chars, tag regex pattern, empty input, mutual-exclusion
• Update success: empty tags array clears all tags
• mint_link rejection: label > 500, max_sessions > 1000, mutual-exclusion
• delete q_ prefix rejection
• batch_create per-item validation with index attribution
• batch_create missing target_url surfaces offending index
• Async batch_create empty/>100 (symmetry with sync)
• batch_create HTTP 400 passthrough with per-item errors
• batch_create still raises on 401 (passthrough is surgical)
• Error type/instance surfacing
• Error detail fallback when RFC 7807 detail missing
• Legacy error.message fallback
• parse_create_output empty qurl_id normalization

BREAKING CHANGE footer (for Release Please)

• Quota.usage.active_qurls_percent is now float | None instead of float = 0.0; callers doing arithmetic must None-check.
• create() no longer accepts an expires_at kwarg — that field wasn't on CreateQurlRequest in the API spec.

Validation

• ruff check — clean
• mypy src/ — 0 errors across 7 source files
• pytest tests/ — 108 passing
• CI workflow YAML validated

[claude]

PR Review: feat!: align types and client with latest API spec

This is a thorough and well-structured PR that aligns the Python SDK with the current OpenAPI spec. The scope is large (+1937/-243 across 11 files, 13 commits) but the changes are cohesive and well-documented. Here's a detailed review:

---

Strengths

• Excellent test coverage expansion — 65 → 108 tests, covering validation boundaries (0 and 1000 for max_sessions), error fallback chains, serialization edge cases (nested dataclasses, datetimes in batch items), and async parity.
• Thoughtful error handling — The RFC 7807 detail fallback chain (detail → message → title → HTTP {status}) is robust and the legacy envelope support is a good defensive measure.
• Client-side validation mirrors API spec constraints with clear error messages that include the offending value, saving round-trips for obvious mistakes.
• CI improvements — Dedicated mypy job pinned to minimum supported Python version is the right call. fail-fast: false provides better visibility.
• Breaking changes are well-documented in commit messages and PR description.

---

Potential Issues

1. allow_statuses on batch_create could silently swallow unexpected 400 shapes

_utils.py:388-414 — If the batch endpoint returns a 400 with a body that isn't the expected BatchCreateOutput shape (e.g., a plain validation error, a proxy error, or malformed JSON), parse_batch_create_output will silently return BatchCreateOutput(succeeded=0, failed=0, results=[]) due to the .get() defaults. The caller gets no indication that something went wrong.

Consider adding a minimal structural check — e.g., verify "results" key exists or "succeeded" is present before treating it as a batch response, and fall back to the normal error path otherwise.

2. QURLError.detail silently replaces the caller's input

errors.py:45-50 — The constructor normalizes detail via message_detail = detail or title, then stores that as self.detail. This means if someone catches the error and inspects .detail, they get the title when the API omitted detail — which is correct for the message string, but means .detail no longer represents the raw API detail field. If downstream code distinguishes "detail was absent" from "detail == title", this conflation could cause issues.

Minor, but consider storing the raw value separately or documenting that .detail is guaranteed non-empty (never reflects the raw API absence).

3. QURLError.type shadows the Python builtin

errors.py:51 — Naming an attribute type shadows Python's built-in type(). This matches RFC 7807 naming but could confuse debugging/introspection (e.g., type(err) inside the class body would refer to the attribute, not the builtin). An alias like problem_type with type as a deprecated property would be cleaner, though this is a style judgment call.

4. update() empty-input check doesn't consider tags=[] as "no update"

client.py:339-346 — The guard if extend_by is None and expires_at is None and description is None and tags is None correctly allows tags=[] through (clearing tags is a valid operation). But passing only tags=[] with build_body will produce {"tags": []} — an empty array that tells the API "clear all tags." If a user calls update("r_abc", tags=[]) intending to do nothing, they'll actually clear tags. The behavior is correct per the API spec, but the docstring says "Pass an empty list to clear" — worth calling out that this is the only way to clear tags, and tags=None means "don't touch tags."

This is well-handled — just confirming it's intentional.

---

Code Quality Observations

5. Sync/async client duplication

client.py and async_client.py have ~200 lines of duplicated logic (validation, body construction, docstrings). This is the standard trade-off in Python async SDKs, but it's a maintenance risk — if a future change updates one client's validation but not the other, they'll silently diverge. Consider a brief comment at the top of each client noting this contract, or a shared private module for the validation + body construction logic.

6. validate_create_input validates length but not URL format

_utils.py:182-198 — target_url is checked for max length (2048) but not for basic URL validity (e.g., presence of a scheme). The API handles this server-side, which is fine, but since you're already doing client-side validation for other fields, a basic target_url.startswith(("http://", "https://")) check could catch another common mistake.

7. Tag regex is hardcoded from the spec

_utils.py:142 — _TAG_PATTERN mirrors the OpenAPI spec's ^[a-zA-Z0-9][a-zA-Z0-9 _-]*$. If the spec evolves, this needs manual updating. A brief comment noting the spec version or date this was derived from would help future maintainers.

---

Test Coverage Gaps

8. No test for _parse_access_policy with ai_agent_policy in a GET response

Tests cover serialization of AIAgentPolicy (outbound request bodies) but not deserialization (parsing an API response containing ai_agent_policy inside a QURL's access_policy). A test that mocks a get() response with a populated ai_agent_policy would close this loop — verifying the _parse_access_policy changes in _utils.py:238-256 actually work end-to-end.

9. No test for datetime objects in list filter parameters

build_list_params handles datetime via .isoformat(), but all list/list_all tests pass strings. Consider adding one test with an actual datetime object to cover the .isoformat() branch.

10. No async test for delete() with q_ prefix rejection

Sync tests cover delete("q_...") raising ValueError, but there's no async equivalent. Given the sync/async parity contract, this should be symmetric.

---

Security

11. require_resource_id_prefix is correctly defense-in-depth

The call order in delete() — validate_id() first (rejecting path traversal), then require_resource_id_prefix() (rejecting wrong ID type) — is correct. The resource_id[:16] truncation in the error message is a good touch for avoiding log injection with very long strings.

No SSRF concerns — target_url validation is the API server's responsibility, not the SDK's.

---

Performance

No concerns. The double iteration in batch_create (validate then serialize) is negligible for ≤100 items. _serialize_value recursion depth is bounded by API payload structure.

---

Minor Nits

• Quota.plan: QuotaPlan = "" defaults to empty string, which doesn't match any Literal value. If the API always returns a plan, this default is never hit; if it doesn't, consumers checking quota.plan == "free" will get a false negative. Consider defaulting to "free" or documenting the empty-string case.
• CreateQURLTool._run in langchain.py still defaults expires_in="24h", meaning LangChain-created QURLs always have an expiry even if the caller doesn't specify one. This differs from the direct client API. Intentional?
• In langchain.py:36-37, the CreateQURLTool.description no longer mentions one_time_use or max_sessions as optional params, but the tool no longer accepts them either — so this is consistent. Just noting that LangChain tool users lose access to those features.

---

Verdict

This is a solid, well-tested PR. The breaking changes are justified by API alignment and clearly documented. The main actionable items are:

1. Add a structural check before parsing batch 400 responses (issue feat: initial Python SDK setup #1) — this is the only item I'd consider blocking.
2. Add a deserialization test for ai_agent_policy in GET responses (issue feat!: align types and client with latest API spec #8) — important for confidence in the parser changes.
3. Add an async delete q_ prefix test (issue chore: enforce supply chain age gating #10) — small gap in symmetric coverage.

Everything else is enhancement-level or style.

🤖 Generated with Claude Code

[justin-layerv]

Latest review addressed — commit c7c9ceb

Ten items from the review. Picked up all the actionable ones including the three gaps the reviewer explicitly called out for before-merge attention.

🔴 Blocking fixed

1. batch_create 400 shape guard. New _validate_batch_create_shape helper in _utils.py verifies that a passthrough 400 body matches the expected BatchCreateOutput envelope: succeeded/failed are int, results is a list, and each entry carries a boolean success discriminant. If the API ever returns 400 with a non-batch body (plain error envelope, proxy error, malformed JSON), batch_create now raises QURLError(status=0, code="unexpected_response") instead of silently producing (succeeded=0, failed=0, results=[]). Same defense-in-depth pattern I applied in qurl-typescript earlier. Wired into both sync and async clients. Two new tests cover the envelope mismatch and the per-entry success discriminant path.

🟡 Strongly recommended fixed

8. _parse_access_policy deserialization test — test_get_response_parses_nested_ai_agent_policy mocks a get() response with a fully-populated ai_agent_policy nested inside a token's access_policy and asserts every field rounds-trips. Closes the deserialization coverage gap the reviewer flagged.

10. Async delete() q_ prefix rejection — test_async_delete_rejects_q_prefix_client_side mirrors the sync test. Sync/async symmetry restored.

🟢 Remaining action items

2. QURLError.detail guarantee — added a .. note:: to the class docstring explaining that .detail is always non-empty because the constructor falls back to title when the API omits detail per RFC 7807. Consumers should use .code / .status / .type to distinguish error cases, not inspect .detail for "was it absent?".

3. QURLError.type shadowing rationale — added a .. note:: documenting why the attribute keeps the RFC 7807 field name despite shadowing Python's type() builtin inside method bodies. Kept for spec parity and consistency with qurl-typescript/qurl-mcp. Shadowing only matters inside QURLError methods, not external code.

6. target_url scheme check in validate_create_input — new _ALLOWED_URL_SCHEMES = ("http://", "https://") tuple and a startswith guard. Catches the most common mistake (forgetting http(s)://) client-side. The server still owns SSRF validation (private ranges, metadata hosts, etc.) — the SDK just catches the obvious typos.

5. Sync/async parity comment — client.py's module docstring now carries the reciprocal of the note already on async_client.py: "NOTE: Business logic mirrors async_client.py — keep both in sync. Input validation, body construction, and error handling must match exactly."

7. Tag regex spec pointer — _TAG_PATTERN comment now explicitly calls out the schema location (UpdateQurlRequest.tags.items.pattern in qurl/api/openapi.yaml) and the keep-in-lockstep contract.

9. Datetime in list filter params test — test_list_serializes_datetime_filter_params_as_isoformat passes an actual datetime(2026, 3, 1) to client.list(created_after=) and asserts the URL-encoded ISO-8601 output. build_list_params' .isoformat() branch now has explicit coverage.

Nit: Quota.plan empty-string default — added a .. note:: documenting that the = "" default exists only so the dataclass can be instantiated with no arguments for tests and bootstrap paths; real /v1/quota responses always return a populated plan string.

⏭️ Deferred (reviewer explicitly non-actionable)

• build(deps-dev): Update langchain-core requirement from <1,>=0.3 to >=0.3,<2 #4 update(tags=[]) clearing semantics — reviewer: "This is well-handled — just confirming it's intentional". Already documented in the update() docstring.
• CreateQURLTool._run default expires_in="24h" — reviewer asked "Intentional?", not "change this". It is intentional; LangChain tool surfaces often have opinionated defaults.
• CreateQURLTool.description missing one_time_use/max_sessions mentions — reviewer: "Just noting that LangChain tool users lose access to those features." No action requested; the intentional simpler LangChain surface is long-standing.

Three existing tests updated for the new URL scheme check

test_422_raises_validation_error, test_400_raises_validation_error, and test_batch_create_partial_failure previously used invalid URLs ("not-a-url", "", "bad") to exercise the API's error-parsing paths. The new client-side scheme check now rejects those pre-flight. Each test now uses a syntactically valid URL that passes client-side validation; the mocked API response is unchanged so the tests still exercise the error parsers they intended to.

Validation

• ruff check src/ tests/ — clean
• mypy src/ — 0 errors across 7 source files
• pytest tests/ — 116 passing (up from 108)

[claude]

PR Review: Align SDK types with current OpenAPI spec

Overall assessment: This is a well-executed, thorough PR. The API alignment work is solid, the sync/async parity is maintained carefully, and the test coverage is excellent. The new batch_create() method, input validation, and RFC 7807 error handling are all well-designed. A few suggestions below, mostly minor.

---

Strengths

• Sync/async parity is meticulously maintained. Every feature, validation, and edge case is implemented identically in both clients, with matching test coverage.
• Client-side validation (validate_create_input, validate_update_input, etc.) catches common mistakes before the API round-trip, with clear error messages and spec-sourced constraints. The bool subclass-of-int guard in _require_max_sessions_in_range is a nice defensive detail.
• Batch create design is well thought out — the HTTP 400 passthrough with shape validation, the per-item error attribution by index, and the defense-in-depth _validate_batch_create_shape guard are all sound patterns.
• Error handling improvements — RFC 7807 type/instance fields, the detail fallback chain (detail → message → title → HTTP {status}), and the envelope.get("error") or {} pattern for explicit-null envelopes are robust.
• Test coverage is exceptional — ~1400 lines of new tests covering validation boundaries, wire-body assertions, error edge cases, pagination filter propagation, POST retry safety, and shape-guard defense tests. The regression guards are well-documented with clear descriptions of what they protect against.
• Security awareness — require_resource_id_prefix only echoes the 2-char prefix (not the full ID), _validate_batch_create_shape doesn't embed raw body in error messages, and the target_url scheme check is a sensible client-side SSRF sanity check.

---

Suggestions

1. Comment verbosity (style, non-blocking)

Many inline comments are significantly longer than the code they annotate. For example:

• parse_create_output has a ~12-line comment explaining the qurl_id empty-string normalization — the logic is 2 lines.
• _validate_batch_create_shape._fail has ~15 lines of comments explaining why status=0 is used.
• build_list_params has a 3-line type annotation comment for what amounts to "status is a string."

These are individually reasonable, but in aggregate they make the code harder to scan. The commit messages, PR body, and tests already capture the "why" well. Consider trimming comments to the essential insight — the tests themselves serve as the durable specification.

2. _serialize_value — consider handling enum.Enum (minor, non-blocking)

_serialize_value handles None, datetime, dataclasses, list, and dict. If AccessPolicy or similar types ever gain Enum fields (e.g., a typed status enum), those would pass through unserializable. Not urgent since all current fields are primitives, but worth a mental note.

3. update() removed access_policy — intentional but worth a migration note (question)

The update() method no longer accepts access_policy. The PR body mentions this indirectly ("removed access_policy param; added tags") but the migration notes section doesn't call it out explicitly. Since access_policy is still accepted on create() and mint_link(), a consumer upgrading might wonder where it went. A one-liner in the migration notes would help: "To change access policy on an existing resource, use mint_link() with the updated policy." (Or whatever the API-level guidance is.)

4. batch_create — allow_statuses=(400,) interaction with 207 (clarification, non-blocking)

The docstring for _raw_request explains that allow_statuses takes precedence over retries, but the batch endpoint also returns HTTP 207 for partial failures. Since 207 < 400, it already falls through the < 400 success path — but it might be worth a one-line comment at the batch_create call site noting that 207 is handled by the normal success path, and only 400 needs the whitelist.

5. _TAG_PATTERN — no anchored end for tags containing special chars (non-blocking, pedantic)

The tag regex ^[a-zA-Z0-9][a-zA-Z0-9 _-]*$ is correctly anchored and mirrors the OpenAPI spec. One observation: tags like "a " (trailing space) would pass validation. If the API trims trailing whitespace server-side, this is fine. If not, a .strip() or trailing-space check could prevent surprises. Very minor — the server is the authoritative validator.

6. CI: actions/checkout and actions/setup-python SHA pinning (positive note)

Good practice pinning actions to commit SHAs with version comments. The Slack notification aggregation logic correctly handles the new type-check gate. The fail-fast: false on the test matrix is a useful change for visibility.

---

Potential issues

A. Quota.plan default "unknown" vs forward-compat QuotaPlan type

QuotaPlan = Literal["free", "growth", "enterprise"] | str allows any string, so "unknown" is valid at runtime. However, a consumer using exhaustive match against the Literal members would silently hit the str fallback for the sentinel value. This is technically correct behavior (the | str escape hatch exists for exactly this reason), but the 17-line docstring could be shortened to: "Defaults to 'unknown' — only occurs when the field is absent from the API response (tests, malformed responses)."

B. active_qurls_percent: float | None — breaking change for arithmetic callers

Changing from float = 0.0 to float | None = None means any existing code doing quota.usage.active_qurls_percent * 100 will now get a TypeError on unlimited plans. The PR body notes this, and the test locks it in, but since this is a pre-1.0 SDK with no live consumers, this is acceptable. Good call adding the test.

---

Verdict

Approve with minor suggestions. The code is well-structured, thoroughly tested, and makes the right trade-offs for a pre-1.0 SDK alignment. The breaking changes are documented and justified. The suggestions above are non-blocking improvements.

🤖 Generated with Claude Code

[justin-layerv]

Latest review addressed — commit f2004f8

3 fix-now items (1 substantive trim + 2 doc additions), 4 reviewer-explicit/spec-verified defers.

Fixed

1. Item 1 + Issue A — Trim accumulated comment verbosity.
Reviewer explicitly called out three examples where comments had grown beyond their load-bearing value — accumulated across review rounds as different reviewers asked "why?", each answer added inline until the comments became harder to scan than the code:

• parse_create_output qurl_id normalization: 12 lines → 4. Keeps the essential insight (asymmetric with label: empty string is never a meaningful identifier but IS a meaningful "cleared" value for user-facing metadata) and drops the historical reviewer context.

• _validate_batch_create_shape._fail: 23 lines → 11. Keeps the three load-bearing facts (DEBUG log carries structural hints not body content; ValidationError subclass for catch-by-class; status=0 is the SDK convention for client-detected failures) and drops the cross-SDK comparison and round-history.

• Quota.plan docstring: 17 lines → 6. Keeps the forward-compat mechanism (QuotaPlan = Literal | str) and the sentinel rationale (only hit by tests/bootstrap paths). Drops the paragraph-long justification for "unknown" vs "" that accumulated across 5 rounds of reviewer ping-pong.

Net diff: -51 / +30 lines across the three spots with zero semantic change. The PR comments, commit messages, and tests already capture the full "why" for anyone who needs deeper context; inline comments now serve current readers rather than review-history archaeology.

2. Item 3 — update() access_policy removal migration note.
The previous migration note was a single sentence; reviewer pointed out it mentioned the removal only indirectly. Expanded to spell out the two supported alternatives with worked code examples:

1. Resource-level policy changes: create a new resource via create() with the new policy. There's no in-place update — the spec's UpdateQurlRequest schema intentionally makes access policy immutable on existing resources.
2. Per-token policy variation: use mint_link(resource_id, access_policy=...) to mint a new access token on an existing resource with a policy override scoped to that specific link; the resource's base policy stays unchanged.

3. Item 4 — batch_create 207 passthrough inline comment.
Added a one-line comment at the allow_statuses=(400,) call site in both sync and async clients explaining why HTTP 207 Multi-Status doesn't need to be whitelisted: 207 flows through the normal status_code < 400 success path automatically. Only the total-failure 400 needs the explicit opt-in because the generic error path would otherwise swallow the populated BatchCreateOutput body. Prevents a future maintainer from wondering "why isn't 207 in the whitelist too?"

Deferred — reviewer-explicit or spec-verified

• Item 2 — _serialize_value enum.Enum handling. Reviewer explicitly: "Not urgent since all current fields are primitives, but worth a mental note." Fourth round this has been raised with identical "forward-looking / not urgent" framing. No current types use enum.Enum; adding handling would be dead code until one does.
• Item 5 — Tag pattern trailing whitespace. Spec-verified multiple rounds ago: server pattern ^[a-zA-Z0-9][a-zA-Z0-9 _-]*$ is identical to the client pattern, and the spec explicitly documents tags are "lowercased and trimmed server-side." Tightening the client would break compatibility with inputs the server accepts.
• Item 6 — CI action SHA pinning. Reviewer positive note, no action.
• Issue B — active_qurls_percent breaking change. Reviewer explicitly: "Good call adding the test." The test locks it in, migration notes cover it.

Pre-existing concern tracked as issue (not fixed in this PR)

Flagged last round but worth referencing: #19 tracks the POST network-error retry duplicate risk. The retry loop catches fetch-level errors with a blanket continue regardless of HTTP method, which risks duplicate item creation on batch_create if a network error hits mid-flight after the server committed. Documented with repro, root cause, and three fix options. Cross-SDK parity: layervai/qurl-typescript#22 tracks the same bug.

Validation

ruff check src/ tests/ → All checks passed!
mypy src/ tests/       → Success: no issues found in 10 source files
pytest tests/ -q       → 135 passed (unchanged — comment and doc changes only)

[claude]

PR Review: Align SDK types and endpoints with current OpenAPI spec

Overall this is a high-quality, well-structured PR. The sync/async parity is maintained carefully, the test coverage is thorough (~1400 new test lines), and the defensive programming choices (batch shape guard, bool-is-int guards, require_resource_id_prefix) are solid. A few items worth discussing:

---

Bug: _raw_request lacks JSON error handling on allow_statuses path

Severity: Medium

In both client.py (~line 662) and async_client.py (~line 648), the allow_statuses success path calls response.json() without exception handling:

if response.status_code < 400 or response.status_code in allow_statuses:
    if response.status_code == 204 or not response.content:
        return None, None
    envelope = response.json()  # <-- no try/except
    return envelope.get("data"), envelope.get("meta")

If a proxy or CDN returns a whitelisted 400 with non-JSON (HTML error page, empty body that passes the response.content truthiness check, etc.), this raises an unhandled ValueError/JSONDecodeError that propagates directly to the caller. The normal error path in parse_error() wraps response.json() in a try/except, but this success-path bypass doesn't get that protection.

The _validate_batch_create_shape guard downstream would catch a malformed JSON body, but it never gets the chance if JSON parsing itself fails. Consider wrapping the response.json() call in a try/except that falls through to parse_error when the allow_statuses response isn't valid JSON.

---

Nit: Comment-to-code ratio

The inline documentation is thorough and clearly written — the _serialize_value None-asymmetry explanation, the allow_statuses precedence-over-retries note, and the build_body load-bearing-falsy warning are all genuinely useful. However, some comments occupy significantly more space than the code they describe (e.g., require_resource_id_prefix has a 15-line docstring + 8-line inline comment for ~5 lines of logic; _validate_batch_create_shape has ~30 lines of comments for ~20 lines of logic). Consider trimming where the code is already self-evident to keep the signal-to-noise ratio high as the codebase grows.

---

Design question: update() drops access_policy parameter

The update() method previously accepted access_policy but this PR removes it, replacing it with tags. If the OpenAPI spec's UpdateQurlRequest intentionally omits access_policy, this is correct. But if access policy updates are done per-token (via mint_link), it might be worth adding a one-line note in the update() docstring pointing users there, since the previous API surface supported it and callers may look for it.

---

Test coverage: strong with a few gaps to consider

The test suite is impressively comprehensive. A few coverage gaps worth noting for follow-up (not blocking):

• Async batch_create shape-guard tests: The sync client has tests for unexpected 400 body shapes (test_batch_create_rejects_unexpected_400_body_shape, test_batch_create_rejects_400_body_with_non_boolean_success, etc.) but no async mirrors. Given the sync/async parity goal, these would be good additions.
• max_sessions=True rejection: The _require_max_sessions_in_range function explicitly guards against bool (since bool is a subclass of int in Python) — a test locking that in would prevent a future simplification from dropping the guard.
• _parse_dt with malformed strings: The parser uses datetime.fromisoformat() which would raise ValueError on garbage strings. It's fine for a trusted API response, but worth a note if the SDK ever needs to parse untrusted data.

---

Minor observations

1. Quota.plan default sentinel: The change from "" to "unknown" is a nice improvement — it makes the "not-populated" state explicit. The parse_quota fallback is aligned correctly.

2. active_qurls_percent nullable change: Good breaking change — the old float = 0.0 default silently masked unlimited plans as "0%". Making it float | None forces callers to handle the case, which is correct.

3. CI improvements: Splitting mypy into its own job and adding fail-fast: false are both sensible changes. The Slack notification aggregation logic handles the new gate cleanly.

4. LangChain tool simplification: Reducing the CreateQURLTool parameters to just target_url, expires_in, and label is a good scope reduction for the agent interface. The skipif marker for missing langchain-core is a nice QoL improvement.

5. _serialize_value recursion: No depth limit, but API payloads are shallow in practice so this is fine. Just flagging for awareness.

---

Summary

This is a well-executed alignment PR. The one actionable item is the missing JSON error handling on the allow_statuses path in _raw_request. Everything else is either a nit or a follow-up item. The test coverage is excellent, and the defensive guards (batch shape validation, bool-is-int checks, r_ prefix enforcement) show careful attention to edge cases.

🤖 Generated with Claude Code

[justin-layerv]

Latest review addressed — commit 73ada8a

4 fix-now items (1 real medium-severity bug, 1 doc note, 2 test gaps), 2 deferrals.

Fixed

1. Bug (medium) — JSON error handling on allow_statuses path.
Real gap the reviewer caught. The success path in _raw_request called response.json() without exception handling, so a proxy/CDN returning a whitelisted 400 with non-JSON content (plain HTML error page, truncated response, plaintext gateway error) would propagate a raw json.JSONDecodeError to the caller — bypassing both the batch shape guard AND the normal parse_error path (which already has its own try/except around JSON parsing).

Fix: wrap the success-path response.json() in try/except that falls through to parse_error(response) on decode failure. parse_error already handles non-JSON error bodies gracefully. raise ... from None suppresses the JSONDecodeError chain since it's noise — the QURLError's own detail captures the relevant failure mode. Applied to both client.py and async_client.py for parity.

Added two regression tests that mock a 400 with <html> body and assert the SDK raises a QURLError with status == 400, not a raw JSONDecodeError:

• test_batch_create_falls_through_to_parse_error_on_non_json_body
• test_async_batch_create_falls_through_to_parse_error_on_non_json_body

2. Design question — update() access_policy docstring note.
Added a "Cannot change access_policy" paragraph to the update() docstrings in both sync and async clients, pointing callers at:

• create() (new resource with new policy — resource-level changes)
• mint_link(access_policy=...) (per-token override — base resource policy unchanged)

The PR description's migration notes already covered this in detail; the docstring note helps callers reading the code directly rather than the PR body.

3. Test gap — Async mirrors of 4 batch shape-guard regression tests.
Sync/async parity gap. Added:

• test_async_batch_create_rejects_unexpected_400_body_shape
• test_async_batch_create_rejects_400_body_with_non_boolean_success
• test_async_batch_create_rejects_400_body_with_bool_counts
• test_async_batch_create_rejects_counts_arithmetic_mismatch

Each mirrors the corresponding sync test exactly. Without these, an async-specific regression (e.g. a refactor that diverged the two code paths) could slip past CI.

4. Test gap — Direct _require_max_sessions_in_range(True) rejection test.
Added test_require_max_sessions_in_range_rejects_bool as a direct unit test of the bool-is-subclass-of-int rejection. The rejection was previously exercised only indirectly via batch bool-counts tests; this direct test makes the intent explicit and would trip immediately if a future refactor simplifies away the isinstance(value, bool) guard. Also sanity-checks that plain int inputs (0, 500, 1000) and None still pass through.

Deferred

• Comment-to-code ratio (3rd raising). The previous round trimmed 51 lines across the three specific spots the previous reviewer called out (parse_create_output, _validate_batch_create_shape._fail, Quota.plan docstring). This round's targets (require_resource_id_prefix, _validate_batch_create_shape) either overlap with what was already trimmed or contain load-bearing "why" context (spec rationale, info-leak posture, load-bearing invariants). Further iterative trimming in response to every review risks losing useful design-decision docs. Holding the line.
• _parse_dt with malformed strings. Reviewer explicit: "worth a note if the SDK ever needs to parse untrusted data." The SDK only parses trusted API responses; adding validation would be speculative hardening.

Validation

ruff check src/ tests/ → All checks passed!
mypy src/ tests/       → Success: no issues found in 10 source files
pytest tests/ -q       → 142 passed (up from 135, +7 new regression tests)

[claude]

PR Review: Align SDK with current OpenAPI spec

Overall assessment: This is a high-quality, well-structured PR. The changes are thorough, well-documented with inline comments explaining rationale, and the test coverage is excellent. A few observations below, organized by category.

---

Code Quality & Best Practices

Strengths:

• Excellent sync/async parity — both client.py and async_client.py are kept in lockstep with shared validation in _utils.py. The module docstring on client.py explicitly calls this out, which is a good practice for maintainability.
• The _serialize_value refactoring in _utils.py is well-designed. The deliberate asymmetry between dataclass None-drop and dict/list None-preservation is clearly documented and correctly handles the API's use of explicit null as a signalling value.
• The build_body / _serialize_value split is clean — top-level None stripping stays in build_body, recursive serialization delegates to _serialize_value.
• The builtins.list import under TYPE_CHECKING to avoid shadowing from the list() method is the correct approach.
• TypedDict split for BatchCreateItem (_BatchCreateItemRequired + total=False subclass) is the right pattern for Python 3.10 compat without typing_extensions.

Minor observations:

• The _TAG_PATTERN regex is compiled at module-import time, which is fine for a single pattern. Good use of re.compile here.
• The require_resource_id_prefix error message avoids leaking the full caller-supplied ID (only echoes the 2-char prefix) — good security hygiene.
• parse_error detail fallback chain (detail → message → title → HTTP {status}) is well thought out and handles both RFC 7807 and legacy envelopes cleanly.

Potential Issues

1. Quota.plan default changed from "" to "unknown": This is a breaking change for any code that checks if not quota.plan: or quota.plan == "". The PR description says no live consumers exist yet, so this is fine for now, but worth noting in the changelog since it changes the dataclass's default sentinel.

2. Usage.active_qurls_percent changed from float = 0.0 to float | None = None: Same observation — callers doing arithmetic on this field without a None check will get a TypeError instead of silently computing with 0.0. This is the correct behavior (the test documents this rationale explicitly), but it's a breaking API surface change. The test test_quota_active_qurls_percent_null documents the intent well.

3. update() "at least one field" validation: The check requires at least one of extend_by, expires_at, description, or tags to be non-None. This is good, but note that calling update("r_abc", tags=None) is equivalent to "not provided" since None is the default — the docstring makes this clear, which is correct.

4. validate_create_input accepts http:// scheme: The URL scheme check allows both http:// and https://. This is intentional per the comment (SSRF protection is server-side), but some SDKs choose to warn on plain http:// — not a bug, just a design decision.

Performance Considerations

• The _validate_batch_create_shape guard iterates all results to check the success discriminant. For the max batch size of 100, this is negligible. No concerns here.
• _serialize_value is recursive but depth is bounded by the dataclass nesting (max ~3 levels: body → AccessPolicy → AIAgentPolicy), so no stack depth concerns.
• Module-level re.compile for _TAG_PATTERN avoids recompilation per call — correct.

Security

• Target URL validation: Client-side scheme check prevents ftp://, file://, data:// URIs from being sent, reducing SSRF surface before the server-side check.
• Error message hygiene: require_resource_id_prefix only echoes the 2-char prefix, not the full ID. _validate_batch_create_shape logs structural hints at DEBUG only. validate_create_input uses repr(target_url)[:40] instead of raw slicing to avoid TypeError on non-string inputs. All good practices.
• allow_statuses bypass: The allow_statuses=(400,) mechanism in _raw_request is well-documented with the caveat that it bypasses retries. Since 400 isn't in any retry set this is safe, and the docstring explicitly warns future callers about the interaction.
• No credential leakage: The existing mask_key utility is preserved. Error paths don't echo request bodies.

Test Coverage

This is the strongest aspect of the PR. The test additions are thorough and well-organized:

• Every new API surface has both sync and async tests (batch_create, date filters, mint_link expanded params, tags, custom_domain).
• Edge cases are covered: empty tags=[] vs tags=None, empty description="", empty qurl_id="" → None normalization, active_qurls_percent=null, plan missing from response.
• Defense-in-depth tests for the batch shape guard cover: unexpected body shape, non-boolean success discriminant, boolean counts (True/False vs integers), arithmetic count mismatch, non-JSON body fallthrough.
• POST retry safety tests lock in that POST doesn't retry on 5xx but does retry on 429.
• RFC 7231 Retry-After: <HTTP-date> fallback is tested for both sync and async.
• The _serialize_value None-asymmetry test (test_serialize_value_none_asymmetry_dataclass_vs_dict) is a great regression guard.
• Validation boundary tests cover max_sessions bool rejection, tag pattern enforcement, mutual exclusivity of expires_in/expires_at, and the r_ prefix enforcement.
• LangChain tests now properly skip when langchain-core is not installed via pytestmark.

One gap to note:

• The AccessPolicy serialization in update() has been removed (the access_policy param was dropped from update()). The old test_access_policy_in_update test was replaced, but there's no negative test asserting that passing access_policy to update() raises a TypeError. This is arguably covered by the Python signature itself (it'll raise TypeError: unexpected keyword argument), so it's not a real gap — just an observation.

CI Changes

• Separating mypy into its own type-check job is the right call — it can't silently vanish if the test matrix is restructured.
• Pinning to Python 3.10 for mypy matches python_version in pyproject.toml.
• The fail-fast: false on the test matrix is a good CI practice for full visibility.
• The Slack notification aggregation logic correctly reflects the worst-of-two-gates status.
• The self-reference pattern (layerv-qurl[langchain] in dev deps) is a clean way to keep the langchain dependency in one place.

Summary

This is a well-executed API alignment PR with strong test coverage and careful attention to edge cases, security, and backward compatibility documentation. The breaking changes are clearly called out in the PR description (and acceptable given the pre-1.0 status). No blocking issues found.

🤖 Generated with Claude Code

[justin-layerv]

Latest review addressed — commit f6d3967

Short round: 1 test addition + 1 PR description update. Reviewer's overall verdict was "no blocking issues found" — most observations were strengths or explicit non-issues.

Fixed

1. Regression tests for update(access_policy=...) immutability (sync + async)

Reviewer flagged this as a "coverage gap" but hedged "arguably covered by the Python signature itself… not a real gap — just an observation."

I still picked it up. The test isn't a tautology if framed as an invariant guard rather than a language-behavior assertion: a future refactor adding **kwargs to update() for forward-compat pass-through would silently accept access_policy again, breaking the spec-enforced immutability contract (UpdateQurlRequest in qurl/api/openapi.yaml intentionally omits access_policy). The tests lock this down independently of the signature. Both tests commented with the intent so future readers don't delete them as redundant.

def test_update_rejects_access_policy_kwarg(client: QURLClient) -> None:
    with pytest.raises(TypeError, match="access_policy"):
        client.update("r_abc", access_policy={"ai_agent_policy": "allow"})

async def test_async_update_rejects_access_policy_kwarg(...) -> None:
    with pytest.raises(TypeError, match="access_policy"):
        await async_client.update("r_abc", access_policy={"ai_agent_policy": "allow"})

2. PR description — Quota.plan default sentinel change

Reviewer flagged this as worth a changelog note:

Quota.plan default changed from "" to "unknown": This is a breaking change for any code that checks if not quota.plan: or quota.plan == "". […] worth noting in the changelog since it changes the dataclass's default sentinel.

The breaking-changes summary already documented Quota.rate_limits and active_qurls_percent but had missed Quota.plan. Added to the top-level summary (mirror treatment of active_qurls_percent; no migration section needed since the fix is trivial: if not quota.plan or quota.plan == "unknown").

Validation

• ruff check clean
• mypy src/ clean (7 files)
• pytest tests/ 144 passing (up from 142, +2 from this round)

Deferred

None. All other review observations were either:

• Strengths (sync/async parity, _serialize_value asymmetry design, TypedDict split, batch shape guard, collect-all tag validation, listAll cursor termination, info-leak hygiene, etc.)
• "Intentional per the comment" (http:// scheme allowed, allow_statuses bypass docs, _TAG_PATTERN module-level compile)
• "Correct behavior" (active_qurls_percent None — already documented in PR body migration notes)
• "Not a real gap" (the access_policy test itself — picked up anyway as an invariant guard)

Nothing meaningful to defer this round.

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a well-structured, thorough PR that cleanly aligns the SDK with the latest OpenAPI spec. The test coverage is impressive (121 tests with sync/async parity), the defensive validation is thoughtful, and the migration notes in the description are excellent. A few observations below — mostly minor.

---

Code Quality & Architecture

Strengths:

• Clean separation of concerns: validation helpers in _utils.py, parsing logic co-located with types, both clients importing shared functions.
• The _serialize_value recursion with deliberate None-handling asymmetry (drop on dataclasses, preserve in dicts) is well-reasoned and well-tested.
• The allow_statuses mechanism for batch_create's HTTP 400 passthrough is a clean design that doesn't pollute the general error path.
• The build_list_params refactor from manual if-chains to a declarative dict + comprehension is a nice simplification.
• Good use of TypedDict for BatchCreateItem — zero runtime overhead with full IDE/mypy support.

Observations:

• client.py and async_client.py carry ~380 lines of duplicated business logic each (validation, body construction, docstrings). The module-level comment ("keep both in sync") is honest but brittle. Consider whether a shared mixin or a thin async wrapper pattern could reduce the surface area for drift in a follow-up — not a blocker for this PR since the sync/async mirror tests provide a safety net.
• test_client.py grew to ~2160 lines. Consider splitting by feature area (e.g. test_batch.py, test_validation.py) in a follow-up for maintainability.

---

Potential Bugs / Issues

1. _parse_access_policy doesn't guard against non-dict ai_agent_policy (_utils.py:~283). If the API ever returned "ai_agent_policy": "some_string" or "ai_agent_policy": true, the .get("block_all") call would raise AttributeError. A isinstance(ap, dict) guard before constructing AIAgentPolicy would make this resilient to unexpected API shapes — consistent with the defensive posture shown in _validate_batch_create_shape.

2. QURLError.detail is silently coerced (errors.py:~67). The constructor does message_detail = detail or title, which means detail="" becomes the title string. This is intentional per the docstring, but the detail parameter still has a default of "" — so QURLError(status=500, code="x", title="Server Error") will store detail="Server Error", not detail="". The intent is clear from the docs, but a caller passing detail="" explicitly might be surprised. Consider detail: str | None = None with message_detail = detail if detail else title to distinguish "not provided" from "explicitly empty".

3. QURLError.type shadows the builtin (errors.py:~53). The docstring acknowledges this and explains the cross-SDK naming rationale, which is fair. Just flagging that any future method added to QURLError that needs type() will need builtins.type(...) — a small maintenance gotcha.

---

Security

• Good: require_resource_id_prefix only echoes the 2-char prefix, not the full ID — prevents info leaks in error logs. Test test_delete_error_does_not_leak_full_resource_id locks this in.
• Good: _validate_batch_create_shape deliberately omits the raw body from error messages.
• Good: Client-side URL scheme check (http:// / https:// only) provides a fast-fail for SSRF-adjacent mistakes without duplicating the server's full validation.
• The validate_create_input scheme check accepts http:// (not just https://). This is correct per the spec, but worth considering whether the SDK should warn on plain http:// since the protected resource would be served over an unencrypted channel — though this is arguably the caller's responsibility.

---

Performance

• No concerns. The validation helpers are all O(n) or O(1), the _serialize_value recursion is bounded by dataclass depth, and the batch path does a single HTTP round-trip.
• RETRYABLE_STATUS_POST = {429} (set lookup) for the POST retry guard is appropriately efficient.

---

Test Coverage

Test coverage is thorough. Highlights:

• Sync/async parity tests for every new method and edge case.
• Wire-body assertions (not just parsed-response assertions) catch serialization regressions.
• Shape-guard tests for batch_create cover non-dict body, non-boolean success, bool-typed counts, and arithmetic mismatches.
• Date filter propagation tested across pagination (both pages, not just the first).
• The _serialize_value None-asymmetry unit test is a great regression guard.

Minor gaps (non-blocking):

• No test for batch_create with HTTP 207 Multi-Status (the "partial success" path that flows through status_code < 400). The 200 tests exercise the parser, and the code path is the same, but a 207-specific test would lock in the status code routing.
• No test for create() with expires_at removed — the parameter was dropped, but there's no explicit test asserting TypeError if a caller passes it (similar to test_update_rejects_access_policy_kwarg).
• The _parse_access_policy deserialization test covers the happy path with populated ai_agent_policy but not the null / missing case (where ai_policy should be None).

---

CI Changes

The dedicated type-check job is a good improvement — decoupling mypy from the test matrix prevents the gate from silently vanishing if the matrix is restructured. Pinning to Python 3.10 (minimum supported) aligns strictness with what consumers see.

One minor note: the type-check job pins action SHAs (actions/checkout@de0fac..., actions/setup-python@a309f...) which is great for supply-chain safety, but the test job still uses tag refs (not visible in the diff, but worth checking for consistency).

---

Summary

This is a solid, well-tested PR. The breaking changes are clearly documented, the defensive validation is thorough, and the test coverage provides confidence in the sync/async parity. The issues flagged above are minor and none are merge-blockers. Nice work.

🤖 Generated with Claude Code

[justin-layerv]

Latest review addressed — commit ecb7913

6 fix-now items + 13 new regression tests, 4 deferrals (2 filed as tracking issues).

Fixed

1. _parse_access_policy non-dict ai_agent_policy guard (reviewer issue #1)

Real defensive gap. If the API returned "ai_agent_policy": "some_string" or true, .get("block_all") would raise AttributeError. Added isinstance(ap, dict) guard — consistent with the defensive posture in _validate_batch_create_shape. Non-dict values are now silently treated as "no policy" rather than crashing the response parser.

2. QURLError.detail default signature (reviewer issue #2)

Changed from detail: str = "" to detail: str | None = None with message_detail = detail if detail is not None else title. The old detail or title coercion treated detail="" the same as "not provided" — surprising for callers who explicitly pass empty string. Now:

• detail=None (default) → falls back to title ← same as before
• detail="something" → used as-is ← same as before
• detail="" → stored as empty ← changed (previously coerced to title)

Note: the reviewer's suggested detail if detail else title has the same truthiness coercion as the old code — is not None is the correct implementation to distinguish the cases.

Verified no internal callers pass detail="" and no tests assert the implicit coercion — backward-compatible.

3. batch_create HTTP 207 Multi-Status test (sync + async)

Locks in that 207 flows through the success path (gated by status_code < 400). Without this, a future refactor narrowing the range (e.g. == 200 or < 300) would misclassify partial-success responses as errors.

4. create(expires_at=...) TypeError invariant guards (sync + async)

Parallel to the update(access_policy=...) tests added last round. Locks in the removal of expires_at from create() against a future **kwargs refactor. Callers needing absolute expiry should use create(expires_in=...) + update(expires_at=...).

5. _parse_access_policy null/missing ai_agent_policy test

The happy-path test covers a populated AIAgentPolicy, but the None branch wasn't directly asserted. Two cases: explicit null and key-missing — both yield ai_agent_policy=None.

6. list() client-side limit validation (1-100) — Python-parity follow-up from TS round 11

Per OpenAPI spec (GET /v1/qurls → limit: integer, minimum: 1, maximum: 100, default: 20). Added to build_list_params in _utils.py (shared by both sync and async clients — avoids duplication). Rejects 0, negative, floats, and bool (bool is subclass of int, must be explicitly rejected). Omitted limit passes through as-is for server default (20).

Rebuttal

CI action-pin inconsistency — reviewer noted "the test job still uses tag refs (not visible in the diff, but worth checking for consistency)." This is factually incorrect. Both the test job (line 19-21) and type-check job (line 42-44) use identical SHA-pinned actions:

• actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
• actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0

No inconsistency to fix.

Tests (144 → 157, +13)

• test_batch_create_207_multi_status (sync)
• test_async_batch_create_207_multi_status (async)
• test_create_rejects_expires_at_kwarg (sync)
• test_async_create_rejects_expires_at_kwarg (async)
• test_parse_access_policy_null_ai_agent_policy
• test_parse_access_policy_non_dict_ai_agent_policy_is_ignored
• test_list_rejects_limit_zero
• test_list_rejects_limit_above_100
• test_list_rejects_negative_limit
• test_list_rejects_non_integer_limit
• test_list_rejects_bool_limit
• test_list_accepts_limit_at_boundaries
• test_list_omitted_limit_uses_server_default

Validation: ruff clean, mypy clean (7 files), 157 tests passing.

Deferred (with reasoning + tracking issues)

1. Sync/async duplication refactor (~380 lines each) — reviewer explicitly labeled "not a blocker for this PR since the sync/async mirror tests provide a safety net." Filed as Refactor: reduce sync/async client duplication (~380 lines each) #20 with three candidate patterns (shared mixin, thin async wrapper, code generation) and a note on the status-quo safety net.

2. test_client.py split (~2700 lines) — reviewer explicitly labeled "in a follow-up for maintainability." Filed as Chore: split test_client.py (~2700 lines) into per-feature test modules #21 with a suggested 5-file split and shared fixtures plan.

3. QURLError.type builtin shadow — reviewer just flagged as FYI. The docstring already explicitly acknowledges the shadow (lines 23-29 of errors.py) and explains the cross-SDK naming rationale. No action needed.

4. http:// URL scheme warning — reviewer explicitly said "arguably the caller's responsibility." The spec allows both schemes; adding a warning would be SDK policy, not a spec alignment. Not in scope.

[claude]

PR Review: Align SDK types and methods with current OpenAPI spec

Overall Assessment

This is a well-executed, thorough update that aligns the Python SDK with the current API spec. The code quality is high, with careful attention to defensive programming, security, and sync/async parity. The test suite is comprehensive (~1850 new test lines). A few items need attention before merging.

---

Bug: Three async tests missing @pytest.mark.asyncio

These async test functions will silently not run as coroutines (or be skipped, depending on the pytest-asyncio mode):

1. test_async_batch_create_207_multi_status (tests/test_client.py:1647) — has @respx.mock but is missing @pytest.mark.asyncio
2. test_async_update_rejects_access_policy_kwarg (tests/test_client.py:2052) — missing both @respx.mock and @pytest.mark.asyncio
3. test_async_create_rejects_expires_at_kwarg (tests/test_client.py:2536) — missing @pytest.mark.asyncio

These should be fixed before merge — without the decorator, the async parity these tests are supposed to guard is not actually verified.

---

Code Quality & Design — Positive Notes

• Defensive bool subclass-of-int checks in _require_max_sessions_in_range, _validate_batch_create_shape, and build_list_params are a nice touch. Python's True == 1 trap is a real source of subtle bugs.
• _serialize_value None asymmetry (drop on dataclasses, preserve in dicts/lists) is well-reasoned and well-documented. The docstring and the dedicated regression test (test_serialize_value_none_asymmetry_dataclass_vs_dict) make the intent clear.
• require_resource_id_prefix only echoing the 2-char prefix in error messages — not the full ID — is good security hygiene.
• allow_statuses mechanism for batch_create's 400 passthrough is clean and well-documented, with clear precedence rules over the retry path.
• Shape guard (_validate_batch_create_shape) with arithmetic invariant checking is defense-in-depth done right.
• RFC 7807 error handling with the detail fallback chain (detail → message → title → HTTP {status}) is a solid improvement.

Code Quality — Suggestions

• Comment density is high in places. Many of the inline comments (especially in _utils.py) read more like design-decision documentation than code comments. Consider whether some of these belong in a DESIGN.md or ADR rather than inline. Not a blocker — the comments are accurate and useful — but the signal-to-noise ratio in _validate_batch_create_shape and _serialize_value could be improved.
• Sync/async duplication. The batch_create, update, mint_link, and create methods are essentially copy-pasted between client.py and async_client.py (as noted in the module docstring). This is a known SDK pattern, but the validation + body-construction logic could potentially be extracted into shared helpers in _utils.py to reduce the surface area for drift. Not a blocker for this PR, but worth tracking.

---

Potential Issue: build_body with tags=[]

The build_body function strips top-level None values but preserves falsy values like [] and "". This is correctly documented as load-bearing behavior for tags=[] (clear all tags) and description="" (clear description). The tests (test_update_wire_body_preserves_tags_empty_list, test_update_wire_body_preserves_description_empty_string) lock this in well. Just flagging that this is a foot-gun for future contributors — the inline comments are doing important work here.

---

Test Coverage — Strong

• Sync/async parity tests throughout
• Boundary value tests for validation (0, 1000, 501, etc.)
• Shape guard tests (bool counts, arithmetic mismatch, non-boolean success)
• RFC 7807 edge cases (missing detail, explicit null error envelope, legacy message field)
• Wire-body assertions (not just response parsing)
• Date filter propagation across pagination (test_list_all_propagates_date_filters_to_every_page)
• POST retry safety (503 not retried, 429 retried)
• Non-JSON body fallthrough on whitelisted statuses

One gap: there are no tests for the _serialize_value datetime branch being triggered through build_body (e.g., a BatchCreateItem with a datetime value in a nested dict). The unit test covers the function directly, but an integration-style test through batch_create with a datetime would close the loop.

---

CI Changes

The dedicated mypy job pinned to Python 3.10 (minimum supported version) is a good change — it prevents the type-check gate from silently vanishing if the test matrix is restructured. The fail-fast: false addition is also correct for a CI matrix.

---

Security

No concerns. The client-side URL scheme check is appropriately scoped (just http:// and https:// prefix — no attempt to duplicate SSRF protection). Error messages don't leak sensitive data. The allow_statuses mechanism doesn't bypass auth-error handling.

---

Summary

Strong PR. Fix the three missing @pytest.mark.asyncio decorators and this is ready to merge.

[justin-layerv]

Latest review addressed — commit 818919b

Short round. Reviewer's verdict: "Strong PR. Fix the three missing @pytest.mark.asyncio decorators and this is ready to merge."

Rebuttal: @pytest.mark.asyncio NOT needed

The reviewer's primary finding — that 3 async tests are missing @pytest.mark.asyncio — is factually incorrect.

pyproject.toml:68 configures:

asyncio_mode = "auto"

With asyncio_mode = "auto", pytest-asyncio automatically marks all async def test_* functions as asyncio tests. No explicit @pytest.mark.asyncio decorator is needed. This is the recommended configuration per the pytest-asyncio docs.

Verified empirically:

$ pytest tests/ -q -k "test_async_batch_create_207 or test_async_update_rejects_access_policy or test_async_create_rejects_expires_at"
...                                                                      [100%]
3 passed, 154 deselected, 1 warning in 0.11s

All 3 tests run and pass. The async parity coverage IS being exercised.

Fixed

_serialize_value datetime integration test (reviewer's test coverage gap)

The unit test (test_serialize_value_datetime) covers the function directly, but there was no integration test passing a datetime object through the full client method pipeline. Added test_update_serializes_datetime_expires_at which:

1. Passes datetime(2026, 4, 1, 0, 0, 0, tzinfo=timezone.utc) to client.update(expires_at=...)
2. Asserts the wire body contains "2026-04-01T00:00:00+00:00"
3. Closes the build_body → _serialize_value → .isoformat() loop

No deferrals

All other reviewer items were either:

• Positive observations (defensive bool checks, _serialize_value asymmetry, shape guard, RFC 7807 handling, security hygiene)
• Explicitly "not a blocker" (comment density, sync/async duplication — already tracked as Refactor: reduce sync/async client duplication (~380 lines each) #20)
• Flagging known behavior with existing test coverage (build_body tags=[] footgun — tests test_update_wire_body_preserves_tags_empty_list and test_update_wire_body_preserves_description_empty_string already lock this in)

Validation: ruff clean, 158 tests passing (up from 157).

[claude]

PR Review: feat!: align types and client with latest API spec

Overall this is a very well-structured PR. The code quality is high, the design decisions are thoroughly documented, and the test coverage is exemplary. Below are my findings organized by category.

---

Strengths

• Test coverage is excellent. 121 tests with thorough sync/async parity, regression guards for subtle invariants (bool-is-int, build_body falsy-value preservation, batch shape guards), and wire-level body assertions. The test comments explaining why each test exists are particularly valuable.
• Defensive programming throughout. The _validate_batch_create_shape defense-in-depth guard, the bool subclass-of-int checks on max_sessions/succeeded/failed/limit, the URL scheme preflight, and the resource_id prefix enforcement on delete() all prevent real-world misuse from producing confusing errors.
• POST retry safety. Restricting POST retries to {429} only (via RETRYABLE_STATUS_POST) is the correct call for a non-idempotent create endpoint. The tests at lines 957-995 lock this in clearly.
• Error hierarchy is well-designed. RFC 7807 Problem Details support with the detail fallback chain (detail → message → title → HTTP {status}), the "error": null guard (or {} vs , {}), and typed subclasses (AuthenticationError, RateLimitError, etc.) make error handling ergonomic for consumers.
• CI improvements are solid. Dedicated mypy job pinned to the minimum supported Python version, fail-fast: false for full matrix visibility, and the Slack notification script's injection mitigation (env vars + jq for JSON construction) are all good practices.
• Security-conscious details. API key masking in __repr__, not leaking full resource IDs in delete error messages (only echoing the 2-char prefix), and the _RESOURCE_ID_RE regex blocking path traversal in IDs.

---

Issues & Suggestions

1. Missing @pytest.mark.asyncio on one async test (minor inconsistency)

test_async_batch_create_207_multi_status at tests/test_client.py:1678 is async def with @respx.mock but lacks @pytest.mark.asyncio, unlike every other @respx.mock async test in the file. With asyncio_mode = "auto" this works fine at runtime, but it breaks the visual pattern established by all other async tests and could confuse future contributors.

Suggestion: Add @pytest.mark.asyncio for consistency.

2. Sync/async duplication maintenance burden (architectural observation, not blocking)

client.py and async_client.py are ~95% identical — the sync _raw_request uses time.sleep while async uses asyncio.sleep, and the async client uses await / aclose(). The top-of-file comment ("keep both in sync") acknowledges this, and the comprehensive async mirror tests enforce parity. This is a known Python SDK pattern, but for a codebase that's actively evolving (this PR touches both files heavily), it's worth flagging that divergence risk grows with each change.

No action needed now, but if the API surface keeps expanding, consider generating one from the other or using a shared template approach in the future.

3. _parse_dt only handles Z suffix — other timezone offsets work via fromisoformat (informational)

_parse_dt in types.py:25-31 replaces trailing Z with +00:00 for Python 3.10 compat (where fromisoformat doesn't accept Z). Other timezone offsets like +05:30 pass through to fromisoformat directly, which is correct. Just confirming this is intentional and well-handled — no action needed.

4. Retry-After HTTP-date handling is a safe fallback (positive observation)

The parse_error function's .isdigit() check on Retry-After intentionally ignores the HTTP-date form and falls back to exponential backoff. The comment at _utils.py:588-592 and the dedicated test at test_client.py:866-909 both document this clearly. This is the right trade-off — parsing HTTP-dates adds complexity and new failure modes for negligible benefit.

5. Tag validation regex allows trailing spaces (spec-correct, worth noting)

_TAG_PATTERN = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9 _-]*$") allows tags like "team engineering" (spaces in the middle) but also "tag " (trailing space). This mirrors the OpenAPI spec's documented pattern, so it's correct, but trailing-space tags could be a UX surprise for SDK consumers. Consider whether a .strip() or trailing-space warning would be a worthwhile client-side courtesy in a follow-up.

6. batch_create per-item validation doesn't cover access_policy internals (acceptable scope)

The per-item validation in batch_create (lines 520-533) checks target_url, label, max_sessions, and custom_domain but doesn't validate access_policy structure within each item. This is consistent with create()'s behavior (which also doesn't validate access_policy client-side), so it's fine — just noting it for completeness. Deep policy validation is properly left to the server.

7. CreateOutput.qurl_id normalization is well-handled

The parse_create_output function normalizes empty-string qurl_id to None (_utils.py:350-351) while preserving empty-string label as-is. The comment explains the asymmetry clearly: "" is never a meaningful identifier but IS a meaningful "cleared" value for user-facing metadata. Good judgment call, and the test at line 2979 locks it in.

---

No Security Concerns Found

• API key is never logged or included in error messages
• Resource IDs are validated against path traversal patterns before interpolation into URLs
• Error messages from require_resource_id_prefix echo only the 2-char prefix, not the full caller-supplied ID
• The target_url scheme check prevents file://, javascript:, etc. from reaching the API
• The Retry-After cap at 30 seconds prevents a malicious server from stalling the client indefinitely
• The Slack notification script in CI properly sanitizes all GitHub context through env vars + jq

---

Verdict

This is a high-quality PR. The breaking changes are well-documented in the PR description with migration examples, the code is clean and defensive, and the test suite is thorough. The issues above are minor — none are blocking.

🤖 Generated with Claude Code