feat!: restructure QURL type — add AccessToken type

[justin-layerv]

Summary

Major version bump (0.1.0 → 1.0.0) to fix the identity confusion between Resources and QURLs in the SDK types.

• Add AccessToken dataclass for per-token details (qurl_id, one_time_use, max_sessions, session_duration, access_policy, use_count)
• Remove phantom fields from QURL: one_time_use (always False), max_sessions (always None), access_policy (always None), qurl_link (only on CreateOutput)
• Add qurl_count and access_tokens to QURL — populated by the enriched API responses from qurl-service PR #261
• Map API field name: parse_qurl() maps the API's qurls array to access_tokens for clarity
• Export AccessToken from package

BREAKING CHANGE: Code referencing qurl.one_time_use, qurl.max_sessions, qurl.access_policy, or qurl.qurl_link will break. However, these were phantom fields — the API never populated them.

Depends on: layervai/qurl-service#261 (deploy first)

Test plan

• pytest tests/test_client.py — 57/57 tests pass
• mypy --strict — passes

🤖 Generated with Claude Code

[claude]

PR Review: feat!: restructure QURL type — add AccessToken type

Overall this is a clean, well-scoped breaking change. The type separation between Resource and Token makes the SDK model match the API much better. A few items to consider:

---

Bug: falsy access_policy is silently dropped

In _parse_access_token (_utils.py:117):

if data.get("access_policy"):
    policy = _parse_access_policy(data["access_policy"])

data.get("access_policy") will evaluate as falsy for an empty dict {}, which the API could plausibly return. If it does, the policy would be silently dropped instead of parsed. Consider using if data.get("access_policy") is not None: if you want to distinguish "absent" from "empty". This same pattern existed in the old code, but worth fixing now that you're touching the area.

---

Minor: or fallback in token parsing could mask bugs

In parse_qurl (_utils.py:138):

raw_tokens = data.get("qurls") or data.get("access_tokens")

If the API returns "qurls": [] (empty list), or will skip it (falsy) and fall through to data.get("access_tokens"). This means an explicit empty array from the API would be ignored in favor of a potentially stale/different access_tokens key if one existed. Consider:

raw_tokens = data.get("qurls") if "qurls" in data else data.get("access_tokens")

This way an explicit "qurls": [] is respected as "no tokens" rather than falling through.

---

Test coverage gaps

1. No test for the "qurls" → access_tokens field mapping. The test fixture uses "access_tokens" directly. Since parse_qurl explicitly maps "qurls" from the API, there should be at least one test that uses the "qurls" key in the mock response to verify the rename mapping works.

2. No test for access_policy inside an AccessToken. The _parse_access_token function has a code path for nested access_policy parsing, but there's no test exercising it. Consider adding a token fixture with an access_policy to cover that branch.

3. The test_access_policy_in_update test was weakened — it no longer asserts the response is parsed correctly (the access_policy assertion was removed). The test now only checks that the request body was serialized correctly and resource_id round-trips. If the API will stop returning access_policy at the QURL level, this is fine, but worth a note.

---

Versioning: manual bump vs Release Please

The PR manually bumps version in pyproject.toml from 0.1.0 → 1.0.0. Per the CLAUDE.md, Release Please drives versioning from conventional commits. The feat!: prefix should trigger a major bump automatically. Having both a manual bump and Release Please could cause a conflict — Release Please may try to create its own version bump commit on top of this. Confirm that this won't fight with the release automation.

---

Nit

AccessToken.status is typed as QURLStatus, which includes literals like "consumed" and "revoked". If token statuses are a different set than resource statuses, a separate type alias would improve type safety. If they happen to share the same set, this is fine.

---

Summary: The design is sound and the refactoring is well-motivated. The main actionable items are (1) adding a test for the "qurls" → access_tokens key mapping, (2) tightening the or fallback to avoid masking an explicit empty array, and (3) confirming the manual version bump won't conflict with Release Please.

🤖 Generated with Claude Code

[claude]

PR Review: feat!: restructure QURL type — add AccessToken type

Overall this is a clean, well-motivated breaking change. The PR description is thorough and the test coverage is solid. A few items worth addressing:

---

Bug: falsy check on access_policy will skip empty dicts

_utils.py:117 — if data.get("access_policy"): uses a truthiness check. If the API ever returns "access_policy": {}, this evaluates to False and the policy is silently dropped. The same pattern existed in the old code, but since you're refactoring this area anyway, consider tightening to:

if data.get("access_policy") is not None:

This also applies to parse_resolve_output (line 175) and parse_quota (lines 192, 203) if you want to be consistent, though those are outside this PR's scope.

---

Nit: slightly redundant conditional in parse_qurl

_utils.py:138:

raw_tokens = data.get("qurls") if "qurls" in data else data.get("access_tokens")

The "qurls" in data check followed by data.get("qurls") is two lookups for the same key. A minor readability alternative:

raw_tokens = data.get("qurls", data.get("access_tokens"))

This has the same semantics (prefers "qurls" key, falls back to "access_tokens") but is slightly more idiomatic. However, the current form is arguably more explicit about the intent, so this is purely a style preference — fine to keep as-is.

---

Test coverage gap: access_policy on AccessToken

The new _parse_access_token function has a branch for parsing access_policy, but none of the test fixtures include an access_policy on a token. Consider adding a test case (or extending the existing test_get fixture) where one of the tokens in the "qurls" array includes an access_policy dict, to exercise that code path.

---

Minor: no test for _parse_access_token with missing optional fields

The happy-path test provides most fields. It would be good to have a token in the fixture with minimal fields (just qurl_id and status) to verify the defaults (one_time_use=False, max_sessions=0, etc.) are applied correctly. The test_get_without_tokens test covers the case where there are no tokens at all, but not a sparse token.

---

Looks good

• Type modeling is clean — AccessToken as a proper dataclass with sensible defaults.
• The qurls → access_tokens rename mapping is well-documented in both code comments and the PR body.
• Export in __init__.py and __all__ is correct.
• The test_get_without_tokens test is a nice addition for the None tokens path.
• Breaking change is clearly called out.

LGTM with the minor items above addressed (the is not None check being the most important one).

[claude]

PR Review: feat!: restructure QURL type — add AccessToken type

Overall this is a clean, well-structured breaking change. The separation of Resource vs. Token concerns is the right call. A few items to consider:

Code Quality

• Good: Extracted _parse_access_policy and _parse_access_token as focused helper functions — keeps parse_qurl readable.
• Good: Exports are updated in both __init__.py and __all__.
• Good: Dataclass defaults on AccessToken are sensible (e.g. one_time_use=False, counts default to 0).

Potential Issues

1. Inconsistent falsiness check for access_policy (_utils.py:117):
   The new code uses if data.get("access_policy") is not None: — which is correct and handles {} properly. However, parse_resolve_output (line 175) and parse_quota (lines 192, 203) still use the old pattern if data.get("key"): which would silently skip an empty dict {}. Not introduced by this PR, but worth noting the inconsistency — the new pattern is better.

2. status field on AccessToken is a raw string — the QURLStatus type alias is used, but since AccessToken.status is populated directly from data["status"] without validation, any unexpected value passes through silently. This is fine given QURLStatus already unions with str for forward compat, but just calling it out.

3. Field name mapping comment is helpful (_utils.py:137-138) — the "qurls" → access_tokens rename is well-documented inline. One edge case: if the API ever returns both "qurls" and "access_tokens" keys, the current logic would prefer "qurls". This is probably fine, but worth a comment if that scenario is possible.

Test Coverage

• Strong: Three new test cases covering tokens with full fields, tokens with access policy (+ sparse defaults), and the no-tokens case.
• Minor gap: No test for parse_qurl receiving "access_tokens" key directly (the fallback path in line 138). If this SDK-side key name will appear in serialized data (e.g., caching), a test would be good.
• Minor gap: No test for an empty "qurls": [] array — would access_tokens be [] or None? Currently it would be [], which differs from the None when the key is absent. Worth a test to document the intended behavior.

Security

No concerns — no user input flows into the parsing logic unsanitized.

Performance

No concerns — the parsing is O(n) in the number of tokens, which is appropriate.

Summary

Looks good to merge. The breaking change is well-justified (removing phantom fields) and the new AccessToken type correctly models the API's enriched response. Consider adding the two minor test cases above before or after merge.