Enrich audit log entries with changed field details

[Chris0Jeky]

Summary

• Enriches audit log entries for update operations with human-readable change summaries
• BoardService, CardService, ColumnService, and LabelService now log which specific fields changed
• Null/unchanged fields are omitted from the summary

Closes #583

Test plan

• Existing audit tests still pass (20/20)
• New tests verify change details appear in audit entries for each service
• dotnet test passes clean across all projects

[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, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Adversarial self-review

Reviewed: Full diff (git diff main...HEAD), all 4 service files + test file.

Findings

No bugs or security issues found. Specific checks performed:

1. Variable shadowing in CardService: The id lambda parameter in dto.LabelIds.OrderBy(id => id) inside BuildCardChangeSummary is in a static method with no id parameter, so no shadowing issue exists.

2. Edge case: IsBlocked already set to same value: Correctly handled — dto.IsBlocked.Value != oldIsBlocked comparison prevents false-positive logging.

3. Edge case: Labels unchanged but provided: SequenceEqual comparison on sorted lists correctly produces no label-change entry when the same labels are provided.

4. Description redaction: Description values intentionally logged as "Description changed" rather than full text — avoids leaking potentially sensitive content into audit logs.

5. Minor design note: Change summaries detect "DTO field was provided" rather than "actual value changed." This means setting a field to its current value still logs it as changed. This is acceptable and consistent — the DTO expresses user intent, and the domain entity's Update method may or may not be a no-op. The simpler DTO-based approach avoids coupling the audit logic to domain mutation semantics.

6. Null safety: All nullable fields properly guarded. SafeLogAsync resilience wrapper ensures audit failures never crash mutations.

7. Test coverage: 8 new/updated tests covering all 4 services, multi-field changes, no-change edge case, and field-specific assertions. All 20 audit tests pass.

Verdict: Ship-ready.

[gemini-code-assist]

Code Review

This pull request enhances the audit logging system across the Board, Card, Column, and Label services by capturing pre-mutation states and generating detailed change summaries for update operations. The review feedback identifies several opportunities to reduce log noise by ensuring only actual value changes are recorded. Additionally, it highlights a logic error in the card blocking summary and suggests using more specific audit actions for unarchiving boards to improve consistency.

[gemini-code-assist]

The logic for logging a card as "Blocked" is incorrect. It logs the change if dto.IsBlocked.Value is true and different from oldIsBlocked, but it doesn't consider that the card is only actually blocked if !string.IsNullOrEmpty(dto.BlockReason). This can lead to logging a "Blocked" event when the card's state doesn't change, for instance if a request is made with isBlocked: true but no blockReason. The summary-building logic should mirror the update logic in UpdateCardAsync.

        if (dto.IsBlocked.HasValue)
        {
            // A card is only blocked if a reason is given.
            if (dto.IsBlocked.Value && !string.IsNullOrEmpty(dto.BlockReason) && !oldIsBlocked)
            {
                parts.Add($"Blocked: {dto.BlockReason}");
            }
            // A card is unblocked if the flag is set to false and it was previously blocked.
            else if (!dto.IsBlocked.Value && oldIsBlocked)
            {
                parts.Add("Unblocked");
            }
        }

[gemini-code-assist]

When un-archiving a board (dto.IsArchived == false), the audit log action is Updated. For better clarity and consistency, you should use AuditAction.Unarchived, which exists in the AuditAction enum. The change summary already correctly states "Unarchived".

                await SafeLogAsync("board", board.Id, AuditAction.Unarchived, changes: changeSummary);

[gemini-code-assist]

The current implementation of BuildBoardChangeSummary adds a change to the summary if a property in the DTO is not null, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Board' -> 'My Board'. You should check if the value has actually changed before adding it to the summary.

        if (dto.Name != null && dto.Name != oldName)
            parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
        if (dto.Description != null && dto.Description != oldDescription)
            parts.Add($"Description changed");

[gemini-code-assist]

The current implementation adds changes to the summary if properties in the DTO are not null or have a value, without checking if the new value is different from the old one. This can create noisy audit logs (e.g., Title: 'A' -> 'A'). Please add checks to ensure values have actually changed.

        if (dto.Title != null && dto.Title != oldTitle)
            parts.Add($"Title: '{oldTitle}' -> '{dto.Title}'");
        if (dto.Description != null && dto.Description != oldDescription)
            parts.Add("Description changed");
        if (dto.DueDate.HasValue && dto.DueDate != oldDueDate)
            parts.Add($"DueDate: '{oldDueDate?.ToString("O") ?? "none"}' -> '{dto.DueDate.Value:O}'");

[gemini-code-assist]

The current implementation adds a change to the summary if a property in the DTO is not null or has a value, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Column' -> 'My Column'. You should check if the value has actually changed before adding it to the summary.

        if (dto.Name != null && dto.Name != oldName)
            parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
        if (dto.WipLimit.HasValue && dto.WipLimit != oldWipLimit)
            parts.Add($"WipLimit: {oldWipLimit?.ToString() ?? "none"} -> {dto.WipLimit.Value}");
        if (dto.Position.HasValue && dto.Position != oldPosition)
            parts.Add($"Position: {oldPosition} -> {dto.Position.Value}");

[gemini-code-assist]

The current implementation adds a change to the summary if a property in the DTO is not null, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Label' -> 'My Label'. You should check if the value has actually changed before adding it to the summary.

        if (dto.Name != null && dto.Name != oldName)
            parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
        if (dto.ColorHex != null && dto.ColorHex != oldColorHex)
            parts.Add($"Color: '{oldColorHex}' -> '{dto.ColorHex}'");

[Chris0Jeky]

Gemini code review findings - fixed

Fix 1 (HIGH): Card block logic corrected (CardService.cs)

• Block is now logged only when isBlocked=true AND blockReason is non-empty AND the card was not already blocked
• Unblock is logged only when isBlocked=false AND the card was previously blocked
• Removed the fallback "no reason" text that could produce misleading audit entries

Fix 2 (MEDIUM): Noisy logs for unchanged values suppressed (all 4 services)

Added equality checks so BuildChangeSummary methods no longer log a "change" when the DTO value matches the existing value:

• CardService: Title, Description, DueDate now compared against old values
• BoardService: Name, Description now compared against old values
• ColumnService: Name, WipLimit, Position now compared against old values
• LabelService: Name, ColorHex now compared against old values

Fix 3 (MEDIUM): AuditAction.Unarchived used for un-archiving boards (BoardService.cs)

• When dto.IsArchived == false, the audit log now records AuditAction.Unarchived instead of AuditAction.Updated

Tests added (BoardMutationAuditTests.cs)

• UpdateBoard_WithUnarchiveFlag_RecordsUnarchivedAction - verifies AuditAction.Unarchived
• UpdateCard_WithSameTitle_DoesNotLogTitleChange - verifies equality-check suppression
• UpdateCard_BlockWithoutReason_DoesNotLogBlocked - verifies block-without-reason is not logged

Verification

• Full build: 0 errors, 0 warnings
• Full test suite: 1,677 tests passed, 0 failures