feat: type the remaining closed-set params as enums#508
Conversation
Adds a WorkflowCondition enum and a Step.Builder.condition overload; the String overload stays.
Adds a StorageBackend enum and a Builder.backend overload; the String overload stays.
Adds WorkflowCondition; the builder coerces it, and the tracker/saga compare node and run status against the enums instead of string literals.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change introduces enum-backed workflow conditions, storage backends, strategy registration, and webhook delivery statuses across the Java and Python SDKs. Existing wire-string inputs remain supported, while internal workflow and delivery logic uses enum values. ChangesClosed-set enum typing
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
ruff 0.16 flags None not at the end of a type union; reorder the compensates annotations. Annotation-only, no behavior change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/python/taskito/dashboard/delivery_store.py (1)
169-214: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize statuses before persistence and filtering.
A legacy
status="pending"bypasses enum normalization, so Line 194 setscompleted_at; unknown stored statuses likewise fall back toPENDINGonly after Line 214 has already filtered them out. Normalize accepted string inputs and compare filters against_parse_status(row.get("status")).Proposed fix
- status: DeliveryStatus, + status: DeliveryStatus | str, @@ ) -> DeliveryRecord: """Append a delivery row and trim to the per-webhook cap.""" + status = DeliveryStatus(status) now = _now_ms() @@ - if status: - rows = [r for r in rows if r.get("status") == status] + if status: + status = DeliveryStatus(status) + rows = [r for r in rows if _parse_status(r.get("status")) is status]Add coverage for a string
"pending"write and an unrecognized persisted status filtered asPENDING.🤖 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 `@sdks/python/taskito/dashboard/delivery_store.py` around lines 169 - 214, Update the delivery-record write path around DeliveryStore’s append method to normalize string status inputs through the existing status parser before constructing DeliveryRecord, ensuring `"pending"` produces no completed_at and unknown values resolve consistently. In list_for, compare the requested status against _parse_status(row.get("status")) so persisted unrecognized statuses are treated as PENDING during filtering. Add coverage for both string pending writes and filtering an unrecognized persisted status as PENDING.
🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/Taskito.java`:
- Around line 663-669: Rename the new type-safe Builder.backend(StorageBackend)
method to a distinct name so Taskito.builder().backend(null) continues resolving
to the existing String overload. Update its documentation and callers as needed,
and add a compile-compatibility test covering backend(null).
---
Outside diff comments:
In `@sdks/python/taskito/dashboard/delivery_store.py`:
- Around line 169-214: Update the delivery-record write path around
DeliveryStore’s append method to normalize string status inputs through the
existing status parser before constructing DeliveryRecord, ensuring `"pending"`
produces no completed_at and unknown values resolve consistently. In list_for,
compare the requested status against _parse_status(row.get("status")) so
persisted unrecognized statuses are treated as PENDING during filtering. Add
coverage for both string pending writes and filtering an unrecognized persisted
status as PENDING.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 602cd0ff-c73a-44f0-9835-355e76293a1e
📒 Files selected for processing (21)
CHANGELOG.mdsdks/java/src/main/java/org/byteveda/taskito/Taskito.javasdks/java/src/main/java/org/byteveda/taskito/model/StorageBackend.javasdks/java/src/main/java/org/byteveda/taskito/workflows/Step.javasdks/java/src/main/java/org/byteveda/taskito/workflows/WorkflowCondition.javasdks/java/src/test/java/org/byteveda/taskito/model/ClosedSetEnumTest.javasdks/java/src/test/java/org/byteveda/taskito/workflows/WorkflowConditionTest.javasdks/python/taskito/__init__.pysdks/python/taskito/app.pysdks/python/taskito/dashboard/delivery_store.pysdks/python/taskito/dashboard/handlers/webhook_deliveries.pysdks/python/taskito/enums.pysdks/python/taskito/mixins/runtime_config.pysdks/python/taskito/webhooks.pysdks/python/taskito/workflows/__init__.pysdks/python/taskito/workflows/builder.pysdks/python/taskito/workflows/saga/orchestrator.pysdks/python/taskito/workflows/tracker/dag.pysdks/python/taskito/workflows/types.pysdks/python/tests/dashboard/test_webhook_deliveries.pysdks/python/tests/python/test_closed_set_enums.py
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/python/taskito/dashboard/delivery_store.py (1)
169-194: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize legacy string statuses before setting
completed_at.Line 194 treats a valid existing
status="pending"caller as terminal because it is not the enum singleton, persisting a pending record withcompleted_at=now. AcceptDeliveryStatus | strand normalize withDeliveryStatus(status)before constructing the record; add coverage for raw"pending".Proposed fix
- status: DeliveryStatus, + status: DeliveryStatus | str, attempts: int, @@ - now = _now_ms() + status = DeliveryStatus(status) + now = _now_ms()🤖 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 `@sdks/python/taskito/dashboard/delivery_store.py` around lines 169 - 194, Update the delivery-record creation method around DeliveryRecord to accept DeliveryStatus | str and normalize status with DeliveryStatus(status) before constructing the record. Use the normalized enum for both the record’s status and the completed_at pending check, preserving correct behavior for raw "pending" values. Add coverage verifying that a string "pending" status leaves completed_at unset.
🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/workflows/Step.java`:
- Around line 163-170: Update the invalid-condition validation message in
Step.Builder’s string condition path to direct callers to
condition(WorkflowCondition) instead of condition(Condition). Leave the new
Builder.condition(WorkflowCondition) overload and other validation behavior
unchanged.
In `@sdks/python/taskito/workflows/builder.py`:
- Around line 147-148: Align the compensates parameter annotation with the
runtime lookup used by the workflow builder: either resolve HasTaskName objects
through _task_name in the compensator handling, or replace HasTaskName with a
protocol/type requiring name. Ensure every value accepted by the annotation is
accepted by the runtime resolution path.
---
Outside diff comments:
In `@sdks/python/taskito/dashboard/delivery_store.py`:
- Around line 169-194: Update the delivery-record creation method around
DeliveryRecord to accept DeliveryStatus | str and normalize status with
DeliveryStatus(status) before constructing the record. Use the normalized enum
for both the record’s status and the completed_at pending check, preserving
correct behavior for raw "pending" values. Add coverage verifying that a string
"pending" status leaves completed_at unset.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bd21fb5-a1ea-4969-b955-bc56b65c8667
📒 Files selected for processing (21)
CHANGELOG.mdsdks/java/src/main/java/org/byteveda/taskito/Taskito.javasdks/java/src/main/java/org/byteveda/taskito/model/StorageBackend.javasdks/java/src/main/java/org/byteveda/taskito/workflows/Step.javasdks/java/src/main/java/org/byteveda/taskito/workflows/WorkflowCondition.javasdks/java/src/test/java/org/byteveda/taskito/model/ClosedSetEnumTest.javasdks/java/src/test/java/org/byteveda/taskito/workflows/WorkflowConditionTest.javasdks/python/taskito/__init__.pysdks/python/taskito/app.pysdks/python/taskito/dashboard/delivery_store.pysdks/python/taskito/dashboard/handlers/webhook_deliveries.pysdks/python/taskito/enums.pysdks/python/taskito/mixins/runtime_config.pysdks/python/taskito/webhooks.pysdks/python/taskito/workflows/__init__.pysdks/python/taskito/workflows/builder.pysdks/python/taskito/workflows/saga/orchestrator.pysdks/python/taskito/workflows/tracker/dag.pysdks/python/taskito/workflows/types.pysdks/python/tests/dashboard/test_webhook_deliveries.pysdks/python/tests/python/test_closed_set_enums.py
2 participants
Finishes #503. PR #505 shipped splits 1–6 but deliberately left a long tail of inventory
rows; this closes them. Same non-breaking contract as before — every wire and stored value is
byte-identical and existing string callers keep working (Java keeps its
Stringoverloads).What's typed now
WorkflowConditionstep()/gate()coerce a string; the tracker/saga compare against the enum. Java gainsStep.Builder.condition(WorkflowCondition)beside theStringoverload.StorageBackendQueue(backend=…)takes `StorageBackendStrategyregister_type(strategy=…)takes `StrategyDeliveryStatusstatusfield and the dashboard filter are typed; write sites pass enum members.NodeStatus/WorkflowStateworkflows/tracker/dag.pyandworkflows/saga/orchestrator.pycompare node/run status against the enums instead of string literals.Notes
WorkflowCondition,StorageBackendwire forms are the existing lowercase strings, sonothing on the wire or in storage changes.
StorageBackendunwraps to its wire value at theboundary, so the native layer's aliases (e.g.
"postgresql") still work through the raw-string path.WorkflowCondition, backend union, andDeliveryStatusarealready string-literal unions with runtime sets — the house style. One documented divergence
stays:
pendingis a delivery status only Python records (a record exists before its firstattempt settles); other SDKs record only settled attempts.
DeliveryStatusreads fail closed — an unrecognized stored status falls back topendingrather than raising, matching feat: replace stringly-typed closed-set params with enums #505's lenient-read treatment for stored enums.
Verification
ruff+mypyclean (307 source files); workflow, closed-set-enum, and webhookdelivery suites green.
./gradlew buildgreen — spotless, checkstyle, strict javadoc, tests.CHANGELOG.mdextends the existing closed-set entry under Unreleased.Summary by CodeRabbit