Sanitize batch execution failures and scope proposal lookups - #2358
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Exact-head independent review: CLEAN at b07e8dadbe34cdc3e338e532d31ee4657530e02d against c27283fb2926f1bddbbd20ee441808819d70aea4.
No CRITICAL/HIGH finding. The review checked all seven changed files for receipt, persistence, and log disclosure; cleanup failure handling; caller-scoped EF authorization; query-shape parity; sandbox behavior; and revision, revocation, and transaction guards.
Non-blocking: wall-clock samples remain observational because host scheduling varies. The deterministic assertion is equal response content, command count, and normalized query shape.
![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: b07e8dadbe
ℹ️ 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".
| AppliedOperationCount: orderedOperations.Count)); | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception) |
There was a problem hiding this comment.
When the request token is canceled during an awaited transaction operation, OperationCanceledException enters this broad handler. The newly nested recovery catches then consume the cancellation raised by rollback or status persistence using the same canceled token and return UnexpectedError; in the parent implementation, that recovery exception escaped and cancellation propagated. This misclassifies client disconnects and host shutdowns as execution failures, so rethrow caller-requested cancellation before applying generic exception sanitization, using a separate cleanup token if rollback must still run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Classification: MEDIUM, non-blocking. A canceled request can be misclassified, but the changed path does not create a realistic security or data-loss path. Declined for this bounded sanitization PR under the review gate; preserve cancellation semantics in a focused follow-up.
| const int warmupPairs = 2; | ||
| const int measuredPairs = 10; | ||
| var interceptor = new NormalizedCommandShapeInterceptor(); | ||
| using var factory = _factory.WithWebHostBuilder(builder => |
There was a problem hiding this comment.
This derived host retains ProposalHousekeepingWorker, whose immediate and once-per-minute sweep queries AutomationProposals through the same intercepted TaskdeckDbContext. Because the interceptor classifies every such command as proposal-snapshot and each probe clears a process-shared queue immediately before sending its request, an overlapping sweep adds an extra shape and makes the exact single-command assertions fail nondeterministically, especially on a slow CI runner. Build this probe on a worker-disabled host, as the repository's worker-isolated test fixture does, before installing the interceptor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Classification: MEDIUM test-isolation risk, non-blocking. The exact-head focused and hosted runs are green, and this does not affect runtime correctness, security, or data loss. Declined in this PR; isolate the worker in a focused test-harness follow-up.
|
Latest-base proof for exact head
Hosted checks are now re-running for this exact head. No Lane B canonical coordination document edit was introduced. |
|
Final exact-base reconciliation
The hosted gate has restarted for this exact head. No unrelated infrastructure change entered the PR-authored diff. |
|
Third and final base-drift reconciliation
The exact-head hosted gate has restarted. Another base advance will park this PR under the bounded gate-loop rule rather than trigger a fourth sync. |
|
Merged evidence
CLAUDE_SYNC_PACKETshipped facts: Batch-execute unexpected failures now use generic external, persisted, receipt, and log text; proposal authorization snapshots are filtered to caller-readable rows without weakening phase-two ACL, revision, revocation, or transaction checks. |
|
Post-merge checkpoint: the downstream E2E smoke completed successfully at the merged exact head. The late review sweep found no new review or inline thread. |
1 participant
Closes #2281
Summary
UnexpectedError.Evidence
b07e8dadbe34cdc3e338e532d31ee4657530e02d: both paths use 1proposal-snapshotcommand and return the same 404 /NotFound/Proposal not found.response.dotnet test backend/Taskdeck.sln -c Release -m:1 --no-restore: 8661 passed, 5 declared skips, 0 failed.The timing harness retains aggregate durations and normalized command labels only. It does not retain proposal IDs, parameters, response bodies, or raw SQL.
Risk
Strict wall-clock equality is not claimed because host scheduling varies. The deterministic security property is equal response content plus equal command count and normalized query shape for stable missing and unreadable cases. Generic internal error logging intentionally trades exception detail for non-disclosure.
No canonical coordination document changed.
CLAUDE_SYNC_PACKET
docs/STATUS.mdanddocs/IMPLEMENTATION_MASTERPLAN.md; reconcile the REVIVAL-11: Risk-tiered review prioritization + explicit batch-confirm + model-derived confidence #1307 entry inOUTSTANDING_TASKS.mdagainst its remaining closure record.