Make proposal preview match apply for reorder, create-card ids, and structure limits - #1374
Merged
Merged
Conversation
A create-card operation carrying a cardId parameter equal to its targetId took the existing-card validation branch, previewed OK, and was registered as planned, then Apply failed on the duplicate id. Route every create-card op through the new-card-id validation so a collision with an existing card fails at preview with a stable 409, keeping preview == apply (#1370).
ColumnService.ReorderColumnAsync clamps an overshooting target to the end (Math.Min(position, columnCount - 1)), but the proposal diff surfaced the raw requested position, so a reorder-to-99 on a 3-column board previewed 'to position 99' and applied 'move to end'. Compute the same clamp against the current board columns when rendering the reorder diff so preview == apply. Apply-side clamping is unchanged (manual UI callers rely on it). Updates GetProposalDiffAsync_ShouldSurfaceDestinationPosition_ForColumnReorderOperations, which previously used a single-column board with an out-of-range position=2 and asserted the raw value, locking in the divergence; it now uses an in-range destination. Adds a clamp-specific preview test and a ColumnService apply test that both land on the same effective position (#1370).
The policy structure checks (op count <= 50, unique/non-negative sequences, parameters <= 10000 chars) ran before revised-payload diffs and at Apply, but not before original-proposal diffs, so a proposal could preview cleanly and then fail structure validation at Apply. Extract the checks into a shared ProposalOperationStructureValidator (AutomationPolicyEngine now delegates to it, keeping one source of truth) and run it before building an original-proposal diff, returning the same ValidationError Apply returns (#1370 preview == apply).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aligns the automation proposal preview and execution phases by extracting structural validation into a shared ProposalOperationStructureValidator and improving validation for card creation and column reordering. The reviewer identified two important issues: first, the column reorder preview clamping uses a point-in-time snapshot of the column count, which can diverge from the actual applied position if preceding operations in the proposal create or delete columns; second, a potential NullReferenceException could occur in ProposalOperationStructureValidator if operation.Parameters is null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Owner
Author
Adversarial Code ReviewSelf-review of the full diff, focused on the CRITICAL
HIGH
MEDIUM
LOW
Bot Comments Addressed
Summary0 CRITICAL / 0 HIGH / 0 MEDIUM / 1 LOW (intentional degraded-path trade-off, not merge-blocking). No code changes required. All three fixes ship with regression tests that fail without the fix (B and C previewed cleanly before; A previewed the raw overshoot). Verification: build 0 errors; targeted Application.Tests 168 passed; full Application.Tests 3459 passed; Api.Tests (Proposal|Mcp|Automation) 359 passed. CI security/architecture/migration/docs/SAST green; Backend Unit + API Integration running. |
![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: 00d5644a6f
ℹ️ 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".
Owner
Author
Adversarial Review — Fixes AppliedNo code changes were required — the review found no CRITICAL/HIGH/MEDIUM defects. The single LOW item is an intentional, documented trade-off, not a dropped finding.
All findings addressed (fixed, or assessed and explicitly retained with justification). Nothing silently dropped; no out-of-scope issues to seed. CI status: required gate GREEN — Backend Unit (ubuntu+windows), API Integration (ubuntu+windows), Frontend Unit, Backend Architecture, Migration Validation, Docs Governance, CodeQL, SAST, secret scans all pass. E2E Smoke pending (backend-only change; no frontend runtime surface). |
Owner
Author
Consolidated Adversarial Review — two independent lenses + bot findingsTwo independent adversarial reviews (correctness lens, test-design lens) are complete and adjudicated. All three fixes are CONFIRMED correct. Refuted candidates from the correctness lens, recorded for the trail:
Surviving findings (all being fixed in this batch, plus one tracked issue): LOW — test design
LOW — wording
Bot findings (both bot reviews inspected in full)
DispositionFixes 1–4 + the two accepted bot findings land in small commits on this branch; the out-of-scope pair gets one tracked issue referenced from this PR. Fix-evidence comment (finding → commit → verification) follows the push. |
…view Apply sources the created card's id exclusively from operation.TargetId (OperationHandlerRegistry.CreateCardAsync ignores a cardId parameter), so the preview-side collision check must validate exactly that id. Drop the cardId parameter fallback: a create op with a cardId parameter and no targetId is executed with a generated id, so preview no longer rejects it on a collision Apply would never hit. Also reword the ValidateNewCardIdAsync messages to the neutral 'Create card id ...' (chatgpt-codex-connector P2 + coordinator FIX 4 on PR #1374).
… validation A null Parameters (legacy rows / nullable DB data) would throw NullReferenceException in the shared structure validator on both preview and apply. Fail closed with the same ValidationError on both paths instead; chose an explicit rejection over a null-conditional so null cannot silently pass the size check (gemini-code-assist MEDIUM on PR #1374). Adds unit tests for the null rejection and the exactly-MaxOperationCount boundary pass.
… coverage - True parity contract test: parses the destination position out of the rendered diff text and asserts ColumnService applies the reorder to exactly that position on the same board - no shared hardcoded literal, so the #1370 regression class cannot be reintroduced without this test failing. - De-degenerate the in-range preview test: strictly interior position 1 on a 3-column board (position 2 equalled the clamp ceiling, so a degenerate always-render-the-ceiling bug would have passed). - Coverage gaps: single-column board previews 'to position 0' for any request; exactly-50-op proposal previews cleanly through the structure gate; pin the documented degraded path (board-column lookup failure renders the RAW requested position - behavior intentionally unchanged). (Coordinator FIX 1/2/3a/3b/3d on PR #1374.)
The contract validator's negative-position guard for column reorders was uncovered; assert it fails preview with the same 'Invalid position: must be non-negative' error the apply-side handlers produce (coordinator FIX 3c on PR #1374).
Owner
Author
Consolidated Review — Fixes AppliedAll in-scope findings from the two adjudicated lenses and both bot reviews are fixed; the out-of-scope pre-existing pair is tracked in #1376. Head is now
Verification (local, Release): build 0 errors; Zero-skip accounting: 8 fixed, 1 refuted with evidence, 1 tracked (#1376). Nothing dropped. |
This was referenced Jul 17, 2026
Chris0Jeky
added a commit
that referenced
this pull request
Jul 17, 2026
) * Gate proposal diff on expiry and structure to match apply GetProposalDiffAsync never checked ExpiresAt and short-circuited zero-op proposals to a cached DiffPreview (200) or a 404, while apply's AutomationPolicyEngine.ValidatePolicy rejects both (expiry: ValidationError 'Proposal has expired'; empty: ValidationError 'Proposal must contain at least one operation'). Both let a reviewer preview a proposal apply always rejects (#1376 preview == apply, same class as #1370/#1374). Run the executor's structure gate then expiry gate on every diff path (revised, original, and the cached-DiffPreview fast paths) in ValidatePolicy's order, returning the identical ValidationError shape. A non-expired, well-formed proposal's diff is unchanged. * Cover preview==apply parity for expired and zero-op proposal diffs Adds expiry-parity tests (original path with a true ValidatePolicy parity assertion, cached-preview fast path, and revised path) and zero-op rejection tests (with and without a stored preview). Retargets the two tests that locked in the old divergence: the cached-preview happy path now uses a non-empty proposal (byte-identical back-compat), and the former 404 zero-op test now asserts the apply-shaped 400. * fix(automation): MEDIUM reuse domain IsExpired in diff expiry gate The diff expiry helper duplicated the AutomationProposal.IsExpired expression (DateTime.UtcNow > ExpiresAt). Reuse the domain property so there is one source of truth for expiry; semantics and the ValidationError shape are identical to apply's ValidatePolicy (gemini-code-assist MEDIUM on PR #1395). * fix(review): LOW update PaperReviewView diff-contract comments for #1376 The no-op guard and error-handler comments still described the pre-#1376 contract (404 when no stored DiffPreview and no operations). After PR #1395 the diff endpoint runs Apply's gates and returns 400 ValidationError for zero-op ('Proposal must contain at least one operation') and expired ('Proposal has expired') proposals; 404 now only means proposal-not-found. Comment-only, no behavior change (round-2 consumer-lens F1 on PR #1395).
Chris0Jeky
added a commit
that referenced
this pull request
Jul 17, 2026
This was referenced Jul 17, 2026
This was referenced Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1370
Restores the review-first
preview == applyguarantee for three post-merge defects found on PR #1339 (confirmed onmain@d46adf48). Each fix makes what a reviewer approves in a proposal preview equal what Apply executes; three focused commits, one per defect.Defect A — reorder clamp divergence
ColumnService.ReorderColumnAsyncclamps an overshooting target to the end (Math.Min(position, columnCount - 1)), but the proposal diff surfaced the raw requested position — a reorder-to-99 on a 3-column board previewed "to position 99" and applied "move to end".Per the coordinator decision, apply-side clamping is unchanged (manual UI callers rely on it); instead the diff now computes the same clamp against the current board columns.
AutomationProposalService.DescribeOperationReadable(reorder branch) now rendersMath.Min(requestedPosition, columnCount - 1)(falling back to the raw value only when the best-effort board lookup returns no columns).Updated test (locked in the divergence):
GetProposalDiffAsync_ShouldSurfaceDestinationPosition_ForColumnReorderOperationspreviously used a single-column board with an out-of-rangeposition = 2and asserted the raw "to position 2" — encoding preview ≠ apply. It now uses a 3-column board so the destination is in range (preview == apply). AddedGetProposalDiffAsync_ShouldSurfaceClampedEffectivePosition_WhenColumnReorderOvershoots(position 99 → preview "to position 2") andColumnServiceTests.ReorderColumnAsync_ShouldClampOvershootingTargetToEnd(position 99 → applied position 2); together they assert preview == apply == 2.Defect B — create-card id collision
A create-card op whose
cardIdparameter equalled itstargetIdtook the existing-card validation branch (ValidateCardBoardAsync), previewed OK, was registered as planned, then Apply failed on the duplicate id.ProposalOperationContractValidator.ValidateEntityScopeAsyncnow routes every create-card op throughValidateNewCardIdAsync(never the existing-card branch), so a collision with an existing card fails at preview with a stable 409 Conflict — consistent with the neighbouring duplicate-within-proposal / empty-id validators.Added
ValidateAsync_ShouldRejectCreateCardWhoseIdCollidesWithExistingCard_EvenWhenCardIdParameterMatchesTargetId.Defect C — structure checks after preview
The policy structure checks (op count ≤ 50, unique/non-negative sequences, params ≤ 10000 chars — PR #1288) ran before revised-payload diffs and at Apply (
ValidatePolicy), but not before original-proposal diffs, so Apply could fail structure validation after a clean preview.Extracted the checks into a shared
ProposalOperationStructureValidator(AutomationPolicyEngine.ValidateOperationStructurenow delegates to it — one source of truth), andGetProposalDiffAsyncruns it before building an original-proposal diff, returning the sameValidationErrorApply returns.Added
GetProposalDiffAsync_ShouldRejectOriginalProposalViolatingStructureLimits.Scope / constraints
No new endpoints, no EF/model changes, no migrations. Clean Architecture boundaries preserved (Application-only, delegates to Domain
Result/ErrorCodes). Stable HTTP codes preserved (409 for id collisions, 400 for structure/validation).Verification (local, Release)
dotnet build backend/Taskdeck.sln -c Release -m:1— 0 errors (pre-existing warnings only).Taskdeck.Application.Tests(ColumnService | AutomationProposalService | ProposalOperationContractValidator | AutomationPolicyEngine | AutomationExecutorService | ProposalRevision) — 168 passed, 0 failed.Taskdeck.Application.Tests— 3459 passed, 0 failed.Taskdeck.Api.Tests(Proposal | Mcp | Automation) — 359 passed, 0 failed.