Bring Python SDK toward TS parity with drift guard#339
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughAdds a parity manifest and checker, wires the checker into contract-surface and publish workflows, and expands the Python SDK with control-plane, connect, adapter, and writeback modules, plus tests and docs. ChangesSDK parity and Python surface
Sequence Diagram(s)sequenceDiagram
participant ContractSurfaceScript as scripts/check-contract-surface.sh
participant PublishWorkflow as .github/workflows/publish.yml
participant PublishPythonWorkflow as .github/workflows/publish-python.yml
participant SDKParityChecker as scripts/check-sdk-parity.mjs
participant ParityManifest as packages/sdk/parity.json
participant PythonExports as packages/sdk/python/src/relayfile/__init__.py
participant TypeScriptExports as TypeScript SDK source tree
ContractSurfaceScript->>SDKParityChecker: run parity check
PublishWorkflow->>SDKParityChecker: run after npm ci
PublishPythonWorkflow->>SDKParityChecker: run before version bump
SDKParityChecker->>ParityManifest: load capabilities and statuses
SDKParityChecker->>PythonExports: read __all__
SDKParityChecker->>TypeScriptExports: scan exports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f673b7bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if body is not None: | ||
| headers["Content-Type"] = "application/json" | ||
|
|
||
| response = self._client.request( |
There was a problem hiding this comment.
This call path sends exactly one Cloud request and immediately raises on 429/5xx, so both the default RelayfileSetupRetryOptions and any caller-provided retry= value are ignored for create/join/connect/status setup operations. In transient Cloud throttling or 5xx scenarios the new Python setup helper fails even though the advertised TypeScript-parity setup flow retries those responses, making setup automation unnecessarily flaky.
Useful? React with 👍 / 👎.
| for (const filePath of listFiles(srcDir, ".ts")) { | ||
| if (filePath.endsWith(".test.ts") || filePath.endsWith(".d.ts")) { | ||
| continue; | ||
| } | ||
| const source = fs.readFileSync(filePath, "utf8"); | ||
| for (const match of source.matchAll(/\bexport\s+(?:async\s+)?(?:abstract\s+)?(?:class|function|const|interface|type)\s+([A-Za-z_$][\w$]*)/g)) { |
There was a problem hiding this comment.
Collecting exports from every .ts file means the parity guard still passes when a manifest symbol remains exported internally but is removed from the package entrypoint or an exported subpath that consumers can actually import. That defeats the new release guard for public SDK parity; for example, removing a symbol from src/index.ts while leaving its source declaration intact would not be reported before publishing.
Useful? React with 👍 / 👎.
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Review: PR #339 — feat(sdk-python): add parity contract and setup helpersSummaryThis PR adds a TS↔Python SDK parity contract ( The change is cohesive and scoped to the stated purpose (Python SDK parity, issue #338). No lifecycle/reaper/dispatch/broker code is touched. Verification (ran against current checkout)
No mechanical (lint/format/typo/import-order) fixes were needed — the diff is already clean, so no files were auto-edited. FindingsNo blocking issues. Two low-severity observations, left as comments per the no-semantic-edit policy (neither warrants a change in this PR):
Addressed comments
Advisory Notes
The PR is functionally complete and all CI-equivalent checks pass locally. I cannot confirm the live GitHub check/mergeable status from this sandbox, so I'm not declaring it human-ready. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/python/src/relayfile/self_host_connect.py`:
- Around line 62-72: Rename the local variable `input` in
`create_connect_session` to a domain-specific name like `session_input` to avoid
shadowing the builtin and satisfy Ruff. Update the surrounding references in the
same block, including the `CreateConnectSessionInput` construction and the
`connect_provider.create_connect_session(...)` call, so the flow remains
identical but clearer.
- Around line 140-144: Normalize provider names in _normalize_relayfile_provider
the same way RelayfileSetup does so self-host lookups are consistent across
flows. Update the normalization logic to lowercase the stripped provider before
returning it, while keeping the existing empty-value validation. Use the
_normalize_relayfile_provider helper and RelayfileSetup provider lookup path as
the reference points.
In `@packages/sdk/python/src/relayfile/setup.py`:
- Around line 575-599: The token export in mount_env() and agent_invite() can
emit an expired relayfile token because they serialize get_token() directly from
a long-lived WorkspaceHandle. Update these methods to refresh or re-resolve the
token immediately before building the returned payload, using the existing
token-refresh capability on the handle, so RELAYFILE_TOKEN and any related
invite fields always carry a current value.
- Around line 122-128: The connect-session validation flow in
_validate_connect_session is incorrectly allowing a missing connectionId to be
replaced later by workspace_id, which can point wait_for_connection() and
adopt_integration() at the wrong resource. Update the connect-session handling
in setup.py so the requested connection ID is preserved when provided, and if
the payload omits connectionId, raise MalformedCloudResponseError instead of
fabricating one; also make the related connection setup path around the later
connect-session handling use the same rule consistently.
- Around line 505-515: The disconnect_integration method currently accepts
connection_id but never uses it, so callers cannot disconnect a specific
connection. Update the request built in disconnect_integration to include
connection_id when present, or remove the argument from the public API if it is
not needed. Keep the behavior consistent with _normalize_provider, request_json,
and _pending_connections so the DELETE targets the intended integration
connection.
- Around line 90-96: The _read_json helper currently catches Exception around
httpx.Response.json(), which can hide unrelated failures and silently return an
empty object. Update the exception handling in _read_json to catch only
ValueError as the JSON decode fallback, leaving other errors uncaught so they
surface normally.
In `@packages/sdk/python/src/relayfile/writeback_consumer.py`:
- Around line 61-77: The ACK path in WritebackConsumer can fail after
handler.execute has already committed side effects, which leaves the item
unacknowledged and can trigger duplicate reprocessing. Update WritebackConsumer
so both the success ACK in consume and the failure ACK in _ack_failure use
bounded retry with backoff, and treat ACK errors as recoverable instead of
letting polling stop. Make the handler flow idempotent around item.id so a
retried or redelivered writeback does not repeat downstream writes.
In `@packages/sdk/python/tests/test_setup.py`:
- Around line 185-195: Rename the `input` parameters in the test double methods
`create_connect_session` and `get_connection_status` to avoid shadowing Python’s
builtin `input`. Update the parameter name consistently within each method when
reading `connectionId`, keeping the fake provider behavior unchanged.
In `@packages/sdk/python/tests/test_writeback_consumer.py`:
- Around line 17-19: The ack_writeback method in the test helper is shadowing
the Python builtin by using input as a parameter name, which triggers Ruff A002.
Rename the parameter in ack_writeback to a non-builtin name and update the
method body accordingly so the reference to input.item_id and the append call
still use the renamed variable consistently.
In `@scripts/check-sdk-parity.mjs`:
- Around line 11-18: Validate the manifest shape before entering the capability
loop in check-sdk-parity.mjs: the current fallback on manifest.capabilities ??
[] can hide a missing or truncated parity file and skip the gate entirely. Add
an upfront check after parsing manifest to require a non-empty capabilities
array, and fail fast with a clear error before collectTypeScriptExports,
collectPythonAll, or the for-loop over capability entries runs.
- Around line 58-85: The SDK parity check is counting exports from every
TypeScript source file instead of only the public entrypoint, so internal
symbols can incorrectly satisfy the manifest. Update collectTypeScriptExports()
to derive exports from the index.ts entrypoint export graph for the TypeScript
SDK, and have check-sdk-parity.mjs compare capability.tsExports only against
symbols reachable from that public surface rather than scanning all .ts files
under src.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9206428-545e-4cd5-a0e0-a48cf893218a
📒 Files selected for processing (17)
.github/workflows/publish-python.yml.github/workflows/publish.ymlpackages/sdk/PARITY.mdpackages/sdk/parity.jsonpackages/sdk/python/README.mdpackages/sdk/python/src/relayfile/__init__.pypackages/sdk/python/src/relayfile/connection.pypackages/sdk/python/src/relayfile/errors.pypackages/sdk/python/src/relayfile/integration_adapter.pypackages/sdk/python/src/relayfile/self_host_connect.pypackages/sdk/python/src/relayfile/setup.pypackages/sdk/python/src/relayfile/types.pypackages/sdk/python/src/relayfile/writeback_consumer.pypackages/sdk/python/tests/test_setup.pypackages/sdk/python/tests/test_writeback_consumer.pyscripts/check-contract-surface.shscripts/check-sdk-parity.mjs
Review: PR #339 —
|
Setup control plane (setup.py): - request_json now honors RelayfileSetupRetryOptions: bounded retry with exponential backoff on transient (408/425/429/5xx) and httpx transport errors; client errors (e.g. 409) still fail fast. - connect_integration no longer fabricates a connectionId from workspace_id; it reuses the requested id or raises MalformedCloudResponseError. - disconnect_integration now forwards connectionId as a query param. - mount_env/agent_invite export get_or_refresh_token() so long-lived handles never emit an expired token. - _read_json catches ValueError only instead of blind Exception. self_host_connect.py: - _normalize_relayfile_provider lowercases (parity with setup flow). - rename local `input` -> `session_input` (builtin shadowing). writeback_consumer.py: - ACK calls go through bounded retry/backoff (_ack_with_retry) so a transient ACK failure after a committed handler side effect does not stop polling / redeliver; documents handler idempotency keyed by item.id. Parity guard (check-sdk-parity.mjs): - fail fast when the manifest has no capabilities. - require every `both` capability tsExport to be reachable from the default entrypoint (index.ts), catching shared symbols removed from index.ts while left in source. ts-only CLI surface stays validated against the source tree. Tests: rename `input` test params; add retry, missing-connectionId, disconnect-query, and ACK-retry coverage. 94 passed; ruff + parity guard green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/sdk/python/src/relayfile/writeback_consumer.py (1)
103-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRuntimeError on exhausted ACK retries can halt consumption loop.
At Line 103,
_ack_with_retryraises after retries, and that exception escapespoll_once()/start(). A single persistent ACK outage can stop the consumer and leave items unacked for redelivery. Prefer treating this as a recoverable path (record/report and continue, or isolate failure per item) so polling stays alive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/python/src/relayfile/writeback_consumer.py` around lines 103 - 105, The retry exhaustion path in _ack_with_retry currently raises a RuntimeError that can escape poll_once() and start(), stopping the writeback consumer on a persistent ACK failure. Update WritebackConsumer so exhausted ack_writeback failures are handled as a recoverable per-item error instead of bubbling out of the polling loop, and make sure the failure is recorded or reported while polling continues. Keep the fix localized around _ack_with_retry and the poll_once()/start() flow so a single bad item does not halt consumption.
🧹 Nitpick comments (1)
packages/sdk/python/tests/test_writeback_consumer.py (1)
111-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for exhausted ACK retries.
Please add a companion case where
fail_times >= ack_max_attemptsto lock expected behavior when retries are exhausted (especially whether polling should continue or fail-fast).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/python/tests/test_writeback_consumer.py` around lines 111 - 124, The existing `test_writeback_consumer_retries_transient_ack_failures` covers only the successful retry path in `WritebackConsumer.poll_once`; add a companion test using `FlakyAckClient` with `fail_times` greater than or equal to `ack_max_attempts` and assert the expected exhausted-retry behavior. Use the same `WritebackConsumer`, `FlakyAckClient`, and `poll_once` setup to verify whether the consumer continues polling or fails fast after ACK retries are exhausted, and lock that behavior with explicit assertions on ack attempts and outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/sdk/python/src/relayfile/writeback_consumer.py`:
- Around line 103-105: The retry exhaustion path in _ack_with_retry currently
raises a RuntimeError that can escape poll_once() and start(), stopping the
writeback consumer on a persistent ACK failure. Update WritebackConsumer so
exhausted ack_writeback failures are handled as a recoverable per-item error
instead of bubbling out of the polling loop, and make sure the failure is
recorded or reported while polling continues. Keep the fix localized around
_ack_with_retry and the poll_once()/start() flow so a single bad item does not
halt consumption.
---
Nitpick comments:
In `@packages/sdk/python/tests/test_writeback_consumer.py`:
- Around line 111-124: The existing
`test_writeback_consumer_retries_transient_ack_failures` covers only the
successful retry path in `WritebackConsumer.poll_once`; add a companion test
using `FlakyAckClient` with `fail_times` greater than or equal to
`ack_max_attempts` and assert the expected exhausted-retry behavior. Use the
same `WritebackConsumer`, `FlakyAckClient`, and `poll_once` setup to verify
whether the consumer continues polling or fails fast after ACK retries are
exhausted, and lock that behavior with explicit assertions on ack attempts and
outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce00a910-dbff-42db-8eef-22ffb8d876b6
📒 Files selected for processing (8)
packages/sdk/PARITY.mdpackages/sdk/parity.jsonpackages/sdk/python/src/relayfile/self_host_connect.pypackages/sdk/python/src/relayfile/setup.pypackages/sdk/python/src/relayfile/writeback_consumer.pypackages/sdk/python/tests/test_setup.pypackages/sdk/python/tests/test_writeback_consumer.pyscripts/check-sdk-parity.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/PARITY.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/check-sdk-parity.mjs
- packages/sdk/parity.json
- packages/sdk/python/src/relayfile/setup.py
- packages/sdk/python/src/relayfile/self_host_connect.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sdk/python/src/relayfile/writeback_consumer.py (1)
20-29: 📐 Maintainability & Code Quality | 🔵 TrivialImplement
AsyncWritebackConsumerfor full async parity.The
writeback-consumerfeature is marked as"status": "both"inpackages/sdk/parity.json, but the implementation inpackages/sdk/python/src/relayfile/writeback_consumer.pyis currently sync-only (usingtime.sleep). To align with coding guidelines requiring sync and async behavior parity for retry/error handling mechanisms:
- Implement
AsyncWritebackConsumerusing the async client andasyncio.sleep.- Alternatively, if async support is intentionally deferred, update
packages/sdk/parity.jsonto reflect"status": "py-only"with a corresponding justification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/python/src/relayfile/writeback_consumer.py` around lines 20 - 29, The writeback consumer is currently sync-only, but the parity metadata says it should support both sync and async behavior. Update the writeback consumer implementation in writeback_consumer.py by adding an AsyncWritebackConsumer that mirrors WritebackConsumer using the async client path and asyncio.sleep for polling/retry handling, keeping error and ack behavior aligned. If async support is not intended yet, change the writeback-consumer entry in parity.json to py-only and add the required justification there.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/python/src/relayfile/writeback_consumer.py`:
- Around line 60-62: The ACK retry path in writeback_consumer’s item handling is
accumulating duplicate failures for the same completed item; update the logic
around the _ack_success flow so only one latest ACK error is retained per item
instead of appending a new (item_id, error) entry on every exhausted retry. Use
the existing completed-item tracking in WritebackConsumer and the retry/error
collection code in the same consumer path to de-duplicate before storing,
replacing any prior error for that item.
- Around line 74-78: The broad Exception catch in writeback_consumer.py is
intentional for the failure-to-ACK flow, but Ruff needs an explicit
justification or a tighter contract. Update writeback_consumer.py around
WritebackConsumer._consume_item (where handler.execute and _ack_failure are
used) by adding a targeted noqa rationale for the broad catch, or narrow the
handler exception type if the handler API can guarantee a smaller set of
failures, so the lint rule is satisfied without changing the ACK behavior.
---
Nitpick comments:
In `@packages/sdk/python/src/relayfile/writeback_consumer.py`:
- Around line 20-29: The writeback consumer is currently sync-only, but the
parity metadata says it should support both sync and async behavior. Update the
writeback consumer implementation in writeback_consumer.py by adding an
AsyncWritebackConsumer that mirrors WritebackConsumer using the async client
path and asyncio.sleep for polling/retry handling, keeping error and ack
behavior aligned. If async support is not intended yet, change the
writeback-consumer entry in parity.json to py-only and add the required
justification there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea198cda-57ba-4864-9d24-c13793371495
📒 Files selected for processing (2)
packages/sdk/python/src/relayfile/writeback_consumer.pypackages/sdk/python/tests/test_writeback_consumer.py
| if item.id in self._completed_items: | ||
| if self._ack_success(item): | ||
| self._completed_items.discard(item.id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid accumulating duplicate ACK errors for the same item.
A completed item whose ACK keeps failing is retried on every poll, and each exhausted retry appends another (item_id, error) entry. For long-running consumers, this can grow unbounded during an ACK outage. Store one latest error per item or de-duplicate before appending.
Minimal de-duplication direction
+ def _record_ack_error(self, item_id: str, error: Exception) -> None:
+ self.ack_errors = [
+ (failed_item_id, failed_error)
+ for failed_item_id, failed_error in self.ack_errors
+ if failed_item_id != item_id
+ ]
+ self.ack_errors.append((item_id, error))
+
def _ack_with_retry(self, ack: AckWritebackInput) -> bool:
@@
- self.ack_errors.append((ack.item_id, error))
+ self._record_ack_error(ack.item_id, error)
return FalseAlso applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/python/src/relayfile/writeback_consumer.py` around lines 60 -
62, The ACK retry path in writeback_consumer’s item handling is accumulating
duplicate failures for the same completed item; update the logic around the
_ack_success flow so only one latest ACK error is retained per item instead of
appending a new (item_id, error) entry on every exhausted retry. Use the
existing completed-item tracking in WritebackConsumer and the retry/error
collection code in the same consumer path to de-duplicate before storing,
replacing any prior error for that item.
| try: | ||
| handler.execute(item, self._provider) | ||
| except Exception as exc: | ||
| self._ack_failure(item, exc) | ||
| continue |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the intentional broad catch for Ruff.
Catching Exception here is part of the writeback failure-to-ACK flow, so add a targeted noqa rationale or narrow the handler exception contract to keep lint clean.
Minimal lint-safe direction
- except Exception as exc:
+ except Exception as exc: # noqa: BLE001 - handler failures are converted into failed writeback ACKs
self._ack_failure(item, exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| handler.execute(item, self._provider) | |
| except Exception as exc: | |
| self._ack_failure(item, exc) | |
| continue | |
| try: | |
| handler.execute(item, self._provider) | |
| except Exception as exc: # noqa: BLE001 - handler failures are converted into failed writeback ACKs | |
| self._ack_failure(item, exc) | |
| continue |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 76-76: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/python/src/relayfile/writeback_consumer.py` around lines 74 -
78, The broad Exception catch in writeback_consumer.py is intentional for the
failure-to-ACK flow, but Ruff needs an explicit justification or a tighter
contract. Update writeback_consumer.py around WritebackConsumer._consume_item
(where handler.execute and _ack_failure are used) by adding a targeted noqa
rationale for the broad catch, or narrow the handler exception type if the
handler API can guarantee a smaller set of failures, so the lint rule is
satisfied without changing the ACK behavior.
Source: Linters/SAST tools
1 participant
Summary
scripts/check-sdk-parity.mjsVerification
node scripts/check-sdk-parity.mjsscripts/check-contract-surface.shuv run --with ruff ruff check src testsinpackages/sdk/pythonuv run python -m pytestinpackages/sdk/python(89 passed)Closes #338