Repository navigation
fix: validate checkpoint SubType constraints - #851
OKAN-SOFTWARE wants to merge 4 commits into
Conversation
|
Thanks for the first contribution @OKAN-SOFTWARE! I am reviewing the PR as we speak, but as a first pass I'll run an AI reviewer to also see if there are any immediate issues. The reviewer will leave comments with a severity rating. Please also be sure to address them should any appear. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if sub_type is None: | ||
| continue |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_o774exfzpnxilygdtusnmrpaik
[P2] Validate empty wire subtypes before normalization
The HTTP handler parses updates through OperationUpdate.from_dict(), whose data.get("SubType") or None converts an explicit "SubType": "" into None. This branch then skips validation, so an otherwise valid checkpoint succeeds and advances its token instead of returning 400. The new empty-string test constructs OperationUpdate directly and misses this path. Validate the supplied wire value before normalization, while preserving support for an omitted subtype, and cover rejection through the HTTP request path.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| if errors: | ||
| reported = errors[:_MAX_VALIDATION_ERRORS] | ||
| count = len(reported) | ||
| noun = "error" if count == 1 else "errors" |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_dayjsbcswrpdsvjjr4hna22gl2
[P3] Revisit the untested singular-error branch
Rule 4, “Every branch has a test that fails without it,” is unmet. The single-error tests check only startswith("1 validation error"), which also accepts "1 validation errors". Always choosing the plural therefore satisfies every added assertion, leaving the singular branch without an observable regression test. Please return to the design phase under rule 8, step 6, and decide whether this formatting distinction belongs in the required behavior and how the agreed test plan will establish it.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
Codex AI reviewFound an HTTP validation gap and two approach findings:
Static review only; tests were not executed. Reviewed commit |
| with pytest.raises(AwsApiException) as exc_info: | ||
| CheckpointValidator.validate_input([_update(sub_type)], _execution()) | ||
| message = str(exc_info.value) | ||
| assert message.startswith(f"{len(constraints)} validation error") |
There was a problem hiding this comment.
These assertions check a prefix and substrings. #836 asks for the exact service message. So a wrong separator, a wrong singular/plural form, or a reordered constraint would still pass here. Could you assert the full string for the one-error case, and for the two-error case in test_multiple_invalid_updates_report_in_input_order (lines 114–117) with the ; join?
Example for order charge:
assert message == (
"1 validation error detected: Value 'order charge' at "
"'updates.1.member.subType' failed to satisfy constraint: "
"Member must satisfy regular expression pattern: [a-zA-Z0-9-_]+"
)| client.checkpoint("execution-arn", "token", [], None) | ||
|
|
||
| assert not exc_info.value.is_retryable() | ||
|
|
There was a problem hiding this comment.
nit: also assert the error category and cause of the exception
assert exc_info.value.error_category is DurableApiErrorCategory.EXECUTION
assert isinstance(exc_info.value.__cause__, ValidationException)
|
Thanks for the detailed review. I've pushed commit
Validation:
The upstream CI workflows are still awaiting authorization. The changes are ready for another review. Thank you! |
|
Thank you very much. We just had a discussion internally amongst the team. Although we can put individual validations in the processor, what we will do instead is a broader redesign where we will implement a consolidated validation layer, so we don't end up playing whack a mole with individual fixes. Please see #466 We will close this PR as this work will happen under #466. Sorry for not communicating this sooner. We have introduced new issue labels "needs-decision" and "not-ready" to communicate whether issues are ready to be implemented in the future. |
|
Thanks for the update and for taking the time to review my contribution. A consolidated validation layer makes sense, especially for maintaining consistency with the service model. I'll follow #466 and would be happy to contribute once the design is finalized. Appreciate the guidance! |
Issue
Fixes #836.
Description of changes
This PR brings the Python local checkpoint server's
OperationUpdate.SubTypevalidation into alignment with the AWS service model.Previously, the local runner could accept subtype values that the AWS service rejects. Consequently, an execution could pass locally but fail when deployed.
Changes implemented:
OperationUpdate.SubTypebefore applying checkpoint updates.[a-zA-Z0-9-_]+pattern.ValidationExceptionwith the invalid value, its 1-based update position, and the violated constraint.FAILED.Root cause and design
The checkpoint validator previously checked operation-state consistency without enforcing the service-model constraints on
SubType.The fix adds validation at the shared checkpoint entry point, protecting requests regardless of how they were constructed.
The in-memory service client also needed to handle the new validation exception consistently with the production client. Without this handling, the local runner retried invalid checkpoint requests until execution timeout instead of treating them as unrecoverable failures.
Scope and design decisions:
SubType, as requested in [Testing]: Local checkpoint server accepts a SubType the service rejects #836.ValidationExceptionrepresents the service's HTTP 400 response.The design follows the service constraints described in #836. Maintainer feedback on the approach is welcome; prior design approval was not obtained.
Regression coverage
Tests cover:
Validation results
git diff --check— passed.The Python testing-package run produced two deprecation warnings but no test failures.
CI status
The upstream Python package, JavaScript examples, and cloud test workflows currently report
action_required. Their results are pending; these statuses should not be interpreted as passing tests.Review status
This PR remains a draft pending outstanding CI and cross-language verification and maintainer design feedback.
Contribution checklist
SubTypevalidation.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.