Add proposal side-effect analysis with 7-category breakdown and reversibility window

[Chris0Jeky]

Summary

• Adds domain value objects (SideEffectTone, SideEffectRow, Reversibility, ProposalSideEffects) for the paper deep-review Section III side-effects breakdown
• Implements ISideEffectAnalyzer / SideEffectAnalyzer service that analyzes a proposal's operations to produce a 7-category side-effect table (Cards, Subtasks, Comments, Activity log, Notifications, Webhooks, Calendar) with active/passive tone classification
• Computes per-proposal reversibility window (default 6h, halved for Critical risk) with human-readable summary and description
• Adds GET /api/automation/proposals/{id}/side-effects endpoint with board-level authorization
• 59 new tests (25 domain + 34 application) covering value object construction, validation, equality, tone classification for all 7 categories, webhook detection, reversibility computation, and edge cases

Test plan

• dotnet build backend/Taskdeck.sln -c Release passes (0 errors)
• dotnet test --filter "FullyQualifiedName~SideEffect" -- 60 tests pass
• dotnet test --filter "FullyQualifiedName~Reversibility" -- 31 tests pass
• Domain: value object creation, tone enum, reversibility defaults, equality/hash contracts
• Service: different operation types produce correct active/passive tone for each of 7 categories
• Service: webhook detection via board subscription lookup
• Service: reversibility window computation varies by risk level (Critical = 3h, others = 6h)
• Service: no-operations edge case produces passive-only rows and descriptive reversibility
• Service: proposal not found returns NotFound result
• Authorization reuses existing AuthorizeProposalAsync pattern from controller

Closes #1020

[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 Findings

Tone classification correctness (7 categories)

Category	Tone	Correct?	Notes
Cards	Active when create/move/archive/update/delete/bulk_move; Passive otherwise	Yes	Uses case-insensitive HashSet lookup against known action types
Subtasks	Always Passive	Yes	Feature not yet supported -- correctly hardcoded
Comments	Always Passive	Yes	Proposals never create comments -- correctly hardcoded
Activity log	Active when any operations exist; Passive for empty	Yes	All applied operations create audit entries
Notifications	Active when any operations exist; Passive for empty	Yes	Approval/rejection generates notifications
Webhooks	Active when board has active subscriptions; Passive otherwise	Yes	Queries GetActiveByBoardAsync; also passive when BoardId is null
Calendar	Always Passive	Yes	Integration not yet available -- correctly hardcoded

Reversibility window

Risk Level	Window	Correct?
Low	6h (21,600,000ms)	Yes
Medium	6h	Yes
High	6h	Yes
Critical	3h (10,800,000ms)	Yes -- halved to signal urgency
No operations	6h with "no operations" messaging	Yes

Issues found and fixed (commit be9a111a)

1. Unused import in SideEffectDtos.cs: using Taskdeck.Domain.Entities was imported but never referenced (DTOs use only primitive types). Removed.
2. Dead code in BuildSideEffectRows: hasColumnMutation variable was computed but never referenced in the row construction. Removed.

Issues noted but not actioned

3. userId parameter unused in AnalyzeAsync: The interface accepts userId as specified by the issue requirements, but it is never referenced inside the method body. Authorization is enforced at the controller level before calling AnalyzeAsync, so the parameter is architecturally redundant. Kept in the interface to match the issue spec and because future refinements (e.g., user-specific notification preferences) may need it.

Null safety when proposal has no operations

• proposal.Operations returns _operations.AsReadOnly() which is never null (initialized in constructor as new List<>())
• operations.Count > 0 checks correctly gate Activity log and Notifications tone
• ComputeReversibility has an explicit no-operations block that overrides the risk-level switch
• Empty BuildSideEffectRows input produces 7 all-Passive rows (verified by test)

Authorization

• Endpoint reuses the existing AuthorizeProposalAsync private method which checks board-level read permissions (or proposal ownership for non-board proposals)
• Consistent with the existing /diff endpoint pattern

[gemini-code-assist]

Code Review

This pull request introduces a side-effect analysis feature for automation proposals, adding a new API endpoint, the SideEffectAnalyzer service, and corresponding domain entities and DTOs. The analysis breaks down proposal impacts into seven categories and determines a reversibility posture based on risk levels. Feedback focuses on refining the analysis logic: specifically, utilizing the currently unused hasColumnMutation variable, tightening the hasCardMutation check to verify target types, and ensuring the Webhooks tone is only marked as active when operations actually exist to trigger them.

[gemini-code-assist]

The hasColumnMutation variable is calculated but never used in the 7-category breakdown. Additionally, hasCardMutation is overly broad as it checks action types without verifying the target type, which could lead to false positives if other entity types share action names like 'create' or 'update'. Since the 'Cards' category is the most appropriate place for board structure changes in this 7-row model, I suggest combining them into a hasBoardMutation flag and refining the target type checks.

        bool hasCardMutation = operations.Any(op => 
            string.Equals(op.TargetType, "card", StringComparison.OrdinalIgnoreCase) && 
            CardMutatingActions.Contains(op.ActionType));
        bool hasColumnMutation = operations.Any(op =>
            string.Equals(op.TargetType, "column", StringComparison.OrdinalIgnoreCase) &&
            string.Equals(op.ActionType, "create_column", StringComparison.OrdinalIgnoreCase));
        bool hasAnyOperation = operations.Count > 0;
        bool hasBoardMutation = hasCardMutation || hasColumnMutation;

[gemini-code-assist]

The 'Cards' row should reflect board structure changes like column creation if they are present, especially since hasColumnMutation was explicitly calculated. Updating the description and tone to account for both cards and columns ensures the analysis is comprehensive within the 7-category constraint.

            new(
                "Cards",
                hasBoardMutation
                    ? "Creates, moves, or archives cards or columns on the board"
                    : "No card or column mutations",
                hasBoardMutation ? SideEffectTone.Active : SideEffectTone.Passive),

[gemini-code-assist]

The 'Webhooks' tone is currently set to Active solely based on whether webhooks are configured for the board (hasActiveWebhooks). However, if the proposal contains no operations (hasAnyOperation is false), no webhooks will actually fire. The tone should reflect the actual impact of the proposal. I suggest updating the logic to ensure the tone is only Active when both webhooks are configured and operations exist to trigger them.

            new(
                "Webhooks",
                hasActiveWebhooks && hasAnyOperation
                    ? "Outbound webhooks configured for this board will fire"
                    : (hasActiveWebhooks ? "No operations to trigger webhooks" : "No outbound webhooks configured"),
                hasActiveWebhooks && hasAnyOperation ? SideEffectTone.Active : SideEffectTone.Passive),

[Chris0Jeky]

Adversarial Review -- SideEffectAnalyzer fixes

Issues found and fixed

1. Bug: hasCardMutation does not check TargetType (MEDIUM)

• SideEffectAnalyzer.cs:63 -- The hasCardMutation flag only checked ActionType against CardMutatingActions, without verifying TargetType == "card". Since ProposeCreateColumnExecutor creates operations with actionType: "create" and targetType: "column", and "create" is in CardMutatingActions, a column-only proposal would incorrectly classify the Cards row as Active with the description "Creates, moves, or archives cards on the board" -- which is wrong.
• Fix: Added op.TargetType == "card" (case-insensitive) check to hasCardMutation. Introduced hasColumnMutation and hasBoardMutation flags.

2. Bug: Cards row ignores column mutations (MEDIUM)

• SideEffectAnalyzer.cs:66-73 -- The Cards row only reflected card mutations. Column creation operations were invisible in the side-effect breakdown, giving the user an incomplete picture of board changes.
• Fix: The Cards row now uses hasBoardMutation (card OR column) and displays context-appropriate descriptions: card-only, column-only, or combined.

3. Bug: Webhooks show Active with no operations (MEDIUM)

• SideEffectAnalyzer.cs:99-104 -- If a board has active webhook subscriptions but the proposal has zero operations, the Webhooks row would show Active with "will fire" -- but no webhooks would actually fire for a no-op proposal.
• Fix: Webhook tone is now Active only when hasActiveWebhooks && hasAnyOperation. Added an intermediate description for the edge case.

4. Test bug: Wrong action type in column-only test

• SideEffectAnalyzerTests.cs:153 -- The test AnalyzeAsync_CardsMutation_ShouldBePassive_WhenCreateColumnOnly used ("create_column", "column") as the operation, but the real ProposeCreateColumnExecutor produces ("create", "column"). The test was passing for the wrong reason and masked bug Claude/create feature f 01 qr r as j4 s14advm nn qhp2 la #1.
• Fix: Corrected the test to use the real action type. Updated assertion to match the new behavior (column mutations make the Cards row Active with column-specific text).

New test coverage

• AnalyzeAsync_CardsMutation_ShouldNotTreatColumnCreateAsCardMutation -- verifies target type filtering
• AnalyzeAsync_Cards_ShouldShowBothCardAndColumnMutations -- verifies combined card+column description
• AnalyzeAsync_Webhooks_ShouldBePassive_WhenActiveWebhooksButNoOperations -- verifies the no-ops edge case
• BuildSideEffectRows_CreateColumnOperation_ShouldSetCardsActive -- static test for column mutations
• BuildSideEffectRows_CreateTargetingNonCard_ShouldNotSetCardMutation -- static test for target type filtering
• BuildSideEffectRows_WithWebhooksButNoOps_ShouldSetWebhooksPassive -- static test for webhook edge case

Verification

• dotnet build backend/Taskdeck.sln -c Release -- 0 errors
• dotnet test --filter "FullyQualifiedName~SideEffect" -- 66 tests passed (25 domain, 40 application, 1 API)

[Chris0Jeky]

Fresh Adversarial Review (Round 3)

CRITICAL

1. Double database fetch for proposal (Confidence: 92)
Controller calls AuthorizeProposalAsync (loads proposal), then _sideEffectAnalyzer.AnalyzeAsync loads the same proposal again. Two round-trips per request, and the two fetches could theoretically differ under concurrent mutation.

Fix: Pass the already-loaded proposal entity into AnalyzeAsync.

IMPORTANT

2. Unused userId parameter in ISideEffectAnalyzer.AnalyzeAsync (Confidence: 95) — Never referenced in the method body. Misleads callers into thinking user-scoped analysis happens inside.

3. hasColumnMutation check is overly broad (Confidence: 85) — Matches any column target type regardless of action type. Unlike hasCardMutation which checks both action+target type. Asymmetry is a latent correctness bug when new column operations are added.

4. SideEffectTone enum lacks range validation in SideEffectRow (Confidence: 80) — Casting (SideEffectTone)42 succeeds silently, producing "42" instead of "active"/"passive" in DTO mapping.

5. No API integration test (Confidence: 84) — 66 unit tests but no HTTP pipeline test for the endpoint.

6. ProposalSideEffects domain class is dead code (Confidence: 82) — Never instantiated by any production code. Only used in tests.

Verified Non-Issues

• Security: AuthorizeProposalAsync checks board read permissions correctly
• 7-category tone classification is accurate
• Reversibility window calculation is correct
• Clean architecture boundaries respected