Add reusable action-batch workflows for CLI and MCP#600
Conversation
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: Add reusable action-batch workflows for CLI and MCPI've conducted a comprehensive review of this substantial PR. Here's my assessment: Review Progress
Code Quality Assessment✅ Excellent Architectural AdherenceThe implementation perfectly follows the Platform Architecture guidelines:
✅ Strong Design Patterns
✅ Comprehensive Test CoverageStrong test coverage with well-structured test classes:
Security Review✅ Secure File OperationsThe file storage implementation (
✅ Input ValidationComprehensive validation throughout:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2-speedygonzales #600 +/- ##
=====================================================
+ Coverage 48.56% 50.79% +2.23%
=====================================================
Files 427 473 +46
Lines 27810 31170 +3360
Branches 4903 5359 +456
=====================================================
+ Hits 13505 15832 +2327
- Misses 12573 13380 +807
- Partials 1732 1958 +226 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86f3c2495c
ℹ️ 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".
Sources/Mailozaurr.Cli/CliRunner.cs
Outdated
| if (planNames.Count == 0 && profileIds.Count == 0 && actions.Count == 0 && sortBy == MailMessageActionPlanBatchSortBy.Id && !descending) { | ||
| return null; |
There was a problem hiding this comment.
Honor explicit id sort when listing plan batches
This fast-path returns null for the query when no filters are set and sort is id ascending, which makes the registry skip ApplyBatchQuerySort and keep store order instead. Because the store returns batches ordered by name (FileMailMessageActionPlanBatchStore.GetAllAsync), mail list-plan-batches --sort id (and the equivalent MCP path that uses the same pattern) cannot actually produce ID-ascending results. Keep a non-null query when the caller explicitly asks for id sort so the requested ordering is applied.
Useful? React with 👍 / 👎.
|
Addressed the review note in 9dbb44d: explicit \id\ sort requests for stored plan batches are now preserved in both CLI and MCP query builders, with regression tests covering both surfaces. |
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: Add reusable action-batch workflows for CLI and MCPI've conducted a comprehensive review of this substantial PR. Here's my assessment: Review Progress
Code Quality Assessment✅ Excellent Architectural AdherenceThe implementation perfectly follows the Platform Architecture guidelines:
✅ Strong Design Patterns
✅ Comprehensive Test CoverageStrong test coverage with well-structured test classes:
Security Review✅ Secure File OperationsThe file storage implementation (
✅ Input ValidationComprehensive validation throughout:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dbb44d44c
ℹ️ 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".
| var normalizedMessageIds = messageIds | ||
| .Where(id => !string.IsNullOrWhiteSpace(id)) | ||
| .Select(id => id.Trim()) | ||
| .Distinct(StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
Preserve message-id casing in confirmation token hashing
CreateToken normalizes messageIds with Distinct(StringComparer.OrdinalIgnoreCase), which collapses IDs that differ only by case before hashing. For providers where message IDs are opaque/case-sensitive, a token generated for one set can also validate a different set that only differs by casing, so destructive actions are no longer uniquely bound to the exact previewed messages. Use case-sensitive deduplication (or no deduplication) when building the token payload.
Useful? React with 👍 / 👎.
Summary
Verification