fix(proposals): apply a move into a sparse column instead of throwing at Insert

[Chris0Jeky]

Closes #3025

Summary

A proposal move into a column whose stored card positions are non-contiguous threw at Apply and
rolled the whole proposal back. OperationHandlerRegistry.MoveCardAsync derived the append index
from max(Position) + 1, so a column holding positions 0 and 2 (a deleted middle card - deletion
does not renumber - or a sparse import) produced index 3, and CardService.MoveCardAsync then
called List.Insert(3, ...) on a two-card list. The executor's catch-all turned the
ArgumentOutOfRangeException into UnexpectedError, so the user saw a 500 at execute after the
proposal had already passed approve. Since #3019, preview also released the move's source WIP slot
for that move, so a following restore was approved against capacity the failed move never freed.

Two changes, both staying inside the positioning idiom the codebase already uses:

• OperationHandlerRegistry appends at the occupant count (targetColumn.Cards.Count). That is
  what "one past the last card" means for the list CardService actually rebuilds, and it does not
  depend on the stored positions being dense.
• CardService.MoveCardAsync clamps the insert index to the end of that list -
  Math.Min(dto.TargetPosition, orderedCards.Count), the same guard
  ColumnService.ReorderColumnAsync already applies to column reorders - so no other caller
  (REST POST /cards/{id}/move, CLI, MCP) can drive an overshooting position into an unhandled
  exception either. Negative positions are still refused: Card.SetPosition runs first and its
  DomainException is already mapped to a ValidationError result.

Preview needs no change. With Apply able to perform the move, the #3019 projection that releases
the move's source slot is correct again, and preview and apply agree. The wider gate question -
WIP-checking create/move themselves at preview - is out of scope here and stays tracked as
#3020; #3012 (conflict-detector projection omitting lifecycle effects) is untouched.

Acceptance (from the issue)

• Executor appends by list count; regression with positions 0 and 2.
• Preview/apply parity test for the sparse-column move plus restore combination
  (MoveIntoASparseColumnThenRestore_IsApprovedAtPreview_AndAppliesEndToEnd).
• [n/a] "Alternatively, or additionally, have the preview validator refuse a move whose append
  position is invalid" - not taken: with the executor fixed there is no invalid append position
  left to refuse, and adding a second refusal would re-open the behaviour Validate cumulative WIP when a proposal restores a card behind other operations (#2926) #3019 deliberately
  narrowed. Recorded as a decline, not a silent drop.

Verification

Ran, in the worktree C:/wt/i3025:

• dotnet test backend/tests/Taskdeck.Application.Tests/Taskdeck.Application.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~OperationHandlerRegistryTests|FullyQualifiedName~CardServiceTests" - 72 passed, 0 failed.
• dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~MoveIntoASparseColumnThenRestore" - 1 passed.
• Reproduced red before the fix, which is the point of the parity test: with only the two
  source edits stashed, the same API test fails with
  Expected applied.StatusCode to be OK ... {"errorCode":"UnexpectedError"} but found InternalServerError
  at execute, while approve still returns 200 - the exact "passes approve, fails mid-apply" shape.
  The two Application-level regressions fail with
  System.ArgumentOutOfRangeException : Index must be within the bounds of the List.
• node scripts/check-docs-governance.mjs and node scripts/check-doc-links.mjs - both pass
  (695 Markdown files, 0 broken relative links).
• dotnet test backend/Taskdeck.sln -c Release -m:1 - the once-per-backend-PR full run; result
  posted to this PR as a comment when it finishes.

NOT run: the frontend suite and Playwright E2E (no frontend file is touched); the Postgres
integration tests (they need a live server); any manual/browser check.

Risk

The clamp changes one externally reachable behaviour: POST /api/boards/{boardId}/cards/{cardId}/move
with a targetPosition past the end of the target column used to surface as a 500 and now appends.
No test asserted the 500, appending is what every other overshoot path in the codebase does, and
the alternative - a 400 - would be a new rejection on a request users can make from a drag-and-drop
client whose position arithmetic disagrees with a sparse column. Positions in the target column are
renumbered contiguously by the same pass, exactly as before.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Independent fresh-context adversarial review — triage

One fresh-context read-only review ran against the worktree sources. No CRITICAL, HIGH or
MEDIUM findings; it could not construct a path from the changed lines to wrong board state,
a security issue, or data loss. Four LOW findings, all triaged here, none dropped.

What it confirmed by source-tracing, worth recording because it is the load-bearing part of the
fix: Column.Cards (ColumnRepository.GetByIdWithCardsAsync, unfiltered .Include) filtered to
!IsArchived is exactly the set CardRepository.GetByColumnIdAsync returns
(Where(c => c.ColumnId == columnId && !c.IsArchived)), so a cross-column move now asks for an
exact append index; the index can never undershoot, so List.Insert can never land mid-list and
silently reorder unrelated cards; and there is no unique (ColumnId, Position) index in the
schema (HasIndex("ColumnId") only), so nothing can collide into a constraint violation.

Fixed (acf8b8ebd)

LOW-1 — the move audit row logged the requested position, not the effective one. Accepted and
fixed: before the clamp existed the two were always equal, so this inaccuracy is one my own change
would have introduced into a trail operators read through the audit API. It now logs
card.Position, read after the renumber loop. The target_column=...; position=... shape is
unchanged, so BoardMetricsService.ParseTargetColumnId and ForecastingService.ParseTargetColumnId
still parse it; both suites re-run green.

LOW-3 — the one path where the registry index still overshoots was untested. Accepted. A
proposal move whose columnId is the card's current column counts the mover in Column.Cards
but excludes it from the list CardService rebuilds, so the index is orderedCards.Count + 1 and
the clamp is load-bearing there rather than drift insurance. That path used to throw and roll the
proposal back; it now sends the card to the bottom of its own column, which is what the preview
projection has always assumed (its move branch returns early for a same-column move, treating it
as executable). Pinned by ExecuteOperationAsync_ShouldSendCardToTheBottom_WhenMoveTargetsItsOwnColumn
and spelled out in the handler comment, which previously explained the clamp only as insurance.

Declined, with reasons

LOW-2 — no single test discriminates the registry line from the clamp. Correct, and I confirmed
it by measurement rather than reasoning: with the clamp in place and the registry line reverted to
Max(Position) + 1, both registry tests still pass. Declining the implied change because the
redundancy is the design, not an oversight — the registry line is correctness (ask for the right
index) and the clamp is containment (never let any caller reach an unhandled exception), and each
is documented as such. A test that isolated the registry line would have to observe the MoveCardDto
it builds, which needs a mockable CardService; its methods are not virtual, so
Mock<CardService> runs the real implementation and the registry tests are integration tests of
both layers by construction. Making CardService virtual to enable that is a far larger change
than the defect warrants. All three behavioural tests do fail against the fully unfixed code, which
is the property that matters for a regression.

LOW-4 — the archived-card test pins duplicate positions inside one column. Noted, declined as
out of scope. The archived card keeps its stored placement (that is how restore returns a card to
its original column) while GetByColumnIdAsync excludes it from renumbering, so an active and an
archived card can share a position index. That state is already reachable on main without this
change — archive a middle card, then perform any in-range move in that column — so it is a
pre-existing property of the archive design (#2920/#2932), not something this PR introduces. The
test states it faithfully. If the tie-break on restore is ever considered a defect it belongs in
its own issue against the archive placement model, not here.

Note on the review input

The reviewer was handed a diff generated before the Cards.Count(card => !card.IsArchived)
refinement and the archived-card regression were committed. It read the worktree sources instead
and reviewed the newer, stronger state, which is what the PR head now contains
(acf8b8ebd adds only the two LOW fixes above on top of it).