Skip to content

fix(import): reject conflicting assignee labels before preview or apply - #3014

Merged
Chris0Jeky merged 5 commits into
mainfrom
codex/issue-2980-import-assignee-labels
Sep 12, 2026
Merged

Chris0Jeky merged 5 commits into
mainfrom
codex/issue-2980-import-assignee-labels

Conversation

@Chris0Jeky

@Chris0Jeky Chris0Jeky commented Sep 11, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #2980 once qualified and merged.

The shared import validator now records one ordinal display label per ordinal source key. A conflicting occurrence fails before board/card/column/label/audit construction, whether reached through Preview, typed Apply, or the raw {source, assigneeMappings} Apply wrapper. Choosing Me or Unassigned does not hide the ambiguity.

No automatic name matching, trimming, case folding, identity remapping, endpoint/auth policy, schema, or UI change. Existing bounded-field, missing-mapping, unknown-key, and importer-only target rules remain.

Regression coverage added

18 native Application.Tests cases cover changed/case-changed/whitespace-changed labels, a conflict within one card, both Apply routes with both mapping choices, identical repeats and distinct-card counts, case-distinct keys, archived cards and audit counts, missing maps, and forbidden third-party targets. Every rejected import asserts zero repository Adds, zero SaveChanges/commit, and one rollback.

These tests invoke the real service with mocked repositories; they are not a real-SQLite proof.

Verification / draft gate

The source baseline was checked byte-for-byte against blob 5d8a669ae588471a1cbd42417ca49218bbb3d4b3. The committed production blob 90f0cf3c55efb72f1a2142d806aaf5b40936912a matches the locally prepared patch; only the intended validation block changed.

NOT RUN locally: .NET compilation/tests, full backend solution, repo docs/link checks. This editing environment has no .NET SDK or direct package/repository network. No synthetic C# execution or passing native test result is claimed. Keep draft pending exact-head hosted CI and independent review.

Commands, behavioral contract and handoff: docs/analysis/2026-09-11-import-assignee-label-consistency.md.

Coexistence

Independent of #3010; based on main 4c479a7ff311912ffb96da027f6e525521bb70df. Does not touch assignment eligibility/deactivation/webhooks, CI control paths, or canonical shared docs. The evidence note carries the proposed contract and the landing follow-through for CARD_ASSIGNMENTS/STATUS. No merge, deployment or human-action check-off.


Claude lane qualification (2026-09-12, alpha-product-trust)

The Codex lane released this draft on 2026-09-11; this lane took it over for qualification and landing. The draft gate above is now satisfied, so the PR is marked ready for review.

Ran locally in an isolated worktree:

  • dotnet test backend/tests/Taskdeck.Application.Tests/Taskdeck.Application.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~BoardImportAssigneeConsistencyTests" - 18/18 passed.
  • Attack first: with BoardJsonExportImportService.cs reverted to the origin/main source and only the new test file kept, the class runs 8 failed / 10 passed - all three Preview_RejectsConflictingLabelsBeforeConstructingABoard rows, the same-card conflict, and all four Apply_BothRoutesRejectConflicts_EvenWithExplicitMappings rows go red. That also refutes the "the raw wrapper case passes for the wrong reason" hypothesis: a deserialization failure would return before BeginTransactionAsync, so AssertNoWrites' RollbackTransactionAsync, Times.Once could not hold.
  • ... --filter "...BoardImportAssigneeConsistencyTests|...ExportImport|...BoardImport|...BoardJson" - 103/103 passed, re-run after merging origin/main (so at the post-fix(review): name restores in card side-effect disclosures #3018 base, not only the drafted one).
  • node scripts/check-docs-governance.mjs and node scripts/check-doc-links.mjs - both pass.

Assertions read, not assumed. AssertNoWrites verifies no AddAsync on Boards, Columns, Cards, Labels or AuditLogs, no SaveChangesAsync, no CommitTransactionAsync, and RollbackTransactionAsync exactly once. All three entry points - PreviewBoardAsync, the typed ImportBoardAsync, and the raw {source, assigneeMappings} ImportBoardFromJsonAsync - funnel through ImportBoardCoreAsync, which is where the new check lives. These are mocked-repository application-boundary assertions, as the PR states; they are not a real-SQLite proof.

Added on landing: the two shared-doc follow-throughs the draft deliberately deferred - the one-label-per-key paragraph in docs/product/CARD_ASSIGNMENTS.md and one bounded docs/STATUS.md entry - plus origin/main merged in (one conflict, the STATUS paragraph both sides add). No source or test change from the draft head.

Not run locally: the full backend solution, any real-SQLite or HTTP-level import test, and the frontend suite. Hosted CI at this head is the solution-level evidence.

Refs #2980. Share ordinal label validation across preview and both apply routes; add 18 regression cases and qualification notes.

Copy link
Copy Markdown
Owner Author

Exact-head qualification update for ebb3ecd67e89bb5bb37084f2b92a167ca0b2632a: hosted application-test steps passed on Windows and Linux; Linux backend-unit, docs governance, architecture, migration and container jobs passed. CI run: https://github.com/Chris0Jeky/Taskdeck/actions/runs/34635715287. Remaining checks, including API integration, were still running at observation. The tests were not run locally; these are hosted job/step results, not a complete-CI or live-SQLite proof. This remains draft pending the rest of CI and independent review. Full lane release: #2980 (comment) .

@Chris0Jeky
Chris0Jeky marked this pull request as ready for review September 12, 2026 00:11
@chatgpt-codex-connector

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Owner Author

Review round 1 - triage (Claude lane, alpha-product-trust)

Independent review (fresh context, read-only, given the full patch and issue #2980): no CRITICAL and no HIGH.

It went after the one hypothesis that could have blocked - a valid Taskdeck export now being falsely rejected - and refuted it on the source: the only producer of SourceAssignees is BoardJsonExportImportService.cs:514, new ImportSourceAssigneeDto(a.UserId.ToString(), a.DisplayName), whose label comes from CardService.MapToDto as a.User?.Username ?? "Participant", resolved through CardAssignmentConfiguration's AutoInclude from one User row per UserId. The key is the user id, so labels per key are structurally uniform, and even the degenerate null-navigation case yields the same "Participant" for every occurrence. The shape real exports do produce - several distinct keys sharing one label - is explicitly not rejected and is pinned by Preview_DistinctOrdinalKeysAreNotMergedByNameOrCase. It also confirmed the frontend re-sends the previewed payload (ExportImportView.vue:136), so preview and Apply cannot diverge, and that no OpenAPI fixture, locale string or frontend spec asserts these messages.

The remaining static gap it named honestly: no Api-level round trip exists today for a board with two cards assigned to two different users (CardAssignmentApiTests.cs:353-359 round-trips a single card with a single assignee). That is a pre-existing coverage gap, not something this diff introduces, and it is not a merge blocker under law 2a.

Fixed here (f2daecdc5)

MEDIUM - the new analysis note asserted things that were false in the commit that added it. docs/analysis/2026-09-11-import-assignee-label-consistency.md said "those shared files were not edited in this draft" while the landing commit edits both docs/product/CARD_ASSIGNMENTS.md and docs/STATUS.md, told the reader to keep the PR in draft, and declared qualification outstanding while the STATUS paragraph in the same commit asserts 18 passing tests. Real: a later agent reading docs/analysis/ as the record would re-add the two shared-doc paragraphs or re-park the work as unqualified. The note now dates the NOT RUN sentence to the drafting session, records the executed run, and says the landing commit already made those edits. Docs-only.

MEDIUM (partial) - STATUS self-contradiction. The reviewer is right that docs/STATUS.md still says in bold that "explicit import mapping is delivered" is false, while the routes, the validator, the AddCardAssignments migration and CardAssignmentApiTests are all on main - so that sentence, not the new paragraph, is the stale one. I did not rewrite another lane's delivery claim inside this PR; instead the new paragraph now states the measured scope (which three routes the repaired validator guards) and points at #3036, filed to reconcile that sentence against main.

Tracked, not fixed

MEDIUM - the conflict error names neither the offending key nor the two labels, unlike every sibling message in the same method (Duplicate label name '{...}', Column '{...}' referenced by card '{...}' was not found). On a 400-card export with one trailing-space label variant the user gets a correct but unlocatable rejection. Filed as #3035, which also carries the two LOWs below so they are not silently dropped:

  • the changed error precedence for doubly-invalid payloads (the label conflict now wins over a later rule; still 400/ValidationError, no test asserted the old order);
  • the absence of an HTTP-status or real-SQLite test for this rejection - AssertNoWrites proves "no AddAsync was called", not "no row survives", and Times.Once on a mocked rollback proves a call, not a rollback. The PR body discloses this limit rather than claiming otherwise.

I am deliberately not changing the production error string in this PR: the message is correct, the change would reset the aging window and re-open the CI cycle for a usability improvement the reviewer itself marked non-blocking, and law 2b puts that past the risk budget for this change. #3035 carries the suggested wording.

Informational, no action: layer purity clean (Application-only, test project references Domain/Application only); no missing-test gap for the changed behaviour itself - the one uncovered adjacent shape, a conflicting pair split across three or more cards with an intervening consistent card, is behaviourally identical to the covered two-card case.

Verification after the fix

The fix commit is documentation only, so re-verification is scoped to it: node scripts/check-docs-governance.mjs and node scripts/check-doc-links.mjs pass. Separately, origin/main was merged in after #3018 landed and the import suite was re-run at that base - 103/103 passed - because a moved merge base is a head change for evidence purposes. The service and test sources are byte-identical to the reviewed head.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Base moved twice while this PR was in the gate: origin/main was merged in at 4de5c1de9 (after #3018 landed) and again at 8d4b980e0 (after #3032 landed). Both conflicts were the same shape - docs/STATUS.md, where both sides add a paragraph in the same place; both resolutions keep both paragraphs and change nothing else, and git diff origin/main...HEAD still shows exactly the five intended files. A moved merge base is a head change for evidence purposes, so the import suite was re-run at each base: 103/103 passed both times, and the docs governance and doc-link checks pass. The service and test sources remain byte-identical to the reviewed head, so no new review round is owed.

The Codex connector returned a usage-limit notice here as well (You have reached your Codex usage limits for code reviews), so there is no Codex review to triage on this PR either; under global law 2g the gate is carried by the fresh-context independent review already triaged above.

@Chris0Jeky
Chris0Jeky merged commit f0eb7a3 into main Sep 12, 2026
36 checks passed
@Chris0Jeky
Chris0Jeky deleted the codex/issue-2980-import-assignee-labels branch September 12, 2026 01:28
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Reject inconsistent assignee labels for one Board import source key

1 participant