fix: scope LLM queue list to authenticated user (#508)

[Chris0Jeky]

Summary

• Adds UserId predicate to GET /api/llm-queue/status/{status} and GET /api/llm-queue/stats so each user only sees their own queue items
• Closes BUG: Fresh user sees other users' automation queue data (data isolation failure) #508

Root Cause

GetByStatus and GetQueueStats endpoints fetched records without filtering by the calling user's identity. The GetUserQueue endpoint was already correct; the status and stats endpoints were not.

Fix

• New repository method GetByUserAndStatusAsync(Guid userId, RequestStatus status) replaces the unscoped GetByStatusAsync call in user-facing service methods
• GetQueueByStatusAsync and GetQueueStatsAsync in LlmQueueService now require and use a userId parameter
• Controller extracts userId via TryGetCurrentUserId (the AuthenticatedControllerBase pattern already established) before calling the service
• ProcessNextRequestAsync is intentionally left unscoped — it is a system/worker operation, not a user-facing list

Tests

• GetByStatus_ShouldOnlyReturnCurrentUserRequests: user A creates a pending item; user B queries Pending status and must not see it
• GetQueueStats_ShouldOnlyCountCurrentUserRequests: user A creates an item; user B's PendingCount must not increase
• Existing unit tests updated to use the new GetByUserAndStatusAsync mock signatures (no behavioral change, just signature alignment)

Risk

Low — additive WHERE clause; no schema changes; no behaviour change for single-user setups.

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

Self-Review (Adversarial Pass)

Categories checked:

• All queue endpoints filtered by user (list, count, detail, delete)

  • POST /api/llm-queue — always passes userId to AddToQueueAsync. OK.
  • GET /api/llm-queue/user — calls GetUserQueueAsync(userId). Was already correct. OK.
  • GET /api/llm-queue/status/{status} — was leaking; now fixed with TryGetCurrentUserId + GetQueueByStatusAsync(userId, status). OK.
  • GET /api/llm-queue/stats — was leaking; now fixed with TryGetCurrentUserId + GetQueueStatsAsync(userId). OK.
  • POST /api/llm-queue/{requestId}/cancel — already passes userId and enforces ownership. OK.
  • POST /api/llm-queue/process-next — intentionally unscoped; this is a system/worker-facing operation that processes the next queued item globally. No user isolation needed here; it does not return a user-attributable list. OK.

• Claims extraction handles missing/null userId gracefully

  TryGetCurrentUserId (in AuthenticatedControllerBase) already validates that UserContext.IsAuthenticated, that UserId is not null/whitespace, and that it parses as a Guid. Returns 401 with a well-formed error contract on any failure. No null slipping through.

• Test is not vacuously passing

  GetByStatus_ShouldOnlyReturnCurrentUserRequests: user A creates an item and we assert items.Should().NotContain(item => item.UserId == userA.UserId). If the fix were absent, user B would receive the item and the test would fail. The baseline count is not used here — the assertion is identity-based, not count-based.

  GetQueueStats_ShouldOnlyCountCurrentUserRequests: establishes a baselinePending before user A creates anything, then asserts the count has not changed after user A creates an item. The baseline could theoretically be non-zero (from other tests using the same SQLite database), but the invariant is that it must not increase due to user A's action — this is the correct check.

• No other controllers with similar cross-user leaks in scope of this fix

  Reviewed all endpoints in LlmQueueController. The only unscoped read paths were GetByStatus and GetQueueStats, both now fixed. The ProcessNextRequestAsync path is a system operation and does not return a filterable user list; it is intentionally global.

No additional issues found. Fix is complete.

[gemini-code-assist]

Code Review

This pull request introduces user-based isolation for LLM queue operations, ensuring that users can only access and view statistics for their own requests. Key changes include updating the ILlmQueueService and ILlmQueueRepository to include userId in status and statistics queries, implementing a new GetByUserAndStatusAsync repository method, and adding integration tests to verify data isolation. Feedback focuses on optimizing the GetQueueStatsAsync implementation to avoid multiple database calls and improving query performance by using non-tracking queries for read-only operations.

[gemini-code-assist]

This implementation is inefficient as it makes four separate database calls to get the counts for each status. Each call fetches a list of full LlmRequest entities, which is unnecessary when only the count is needed.

This can be optimized to a single database query. Consider adding a new method to the ILlmQueueRepository that uses a GROUP BY clause to fetch all status counts for a user in one go.

For example, a new repository method could execute a query similar to this:

var statusCounts = await _context.LlmRequests
    .Where(r => r.UserId == userId)
    .GroupBy(r => r.Status)
    .Select(g => new { Status = g.Key, Count = g.Count() })
    .ToDictionaryAsync(g => g.Status, g => g.Count);

This service method would then just call the new repository method and construct the QueueStatsDto from the returned dictionary. This would be much more performant.

[Chris0Jeky]

Fixed: replaced the four GetByUserAndStatusAsync calls with a new GetStatusCountsByUserAsync repository method that uses a single GROUP BY query. The new method returns Dictionary<RequestStatus, int> and GetQueueStatsAsync now builds the DTO from it with .GetValueOrDefault(status, 0) per status.

[gemini-code-assist]

Since this method is used for read-only operations where the entities are mapped to DTOs, it would be more efficient to disable change tracking by adding .AsNoTracking() to the query. This avoids the overhead of setting up change tracking for entities that won't be updated.

    public async Task<IEnumerable<LlmRequest>> GetByUserAndStatusAsync(Guid userId, RequestStatus status, CancellationToken cancellationToken = default)
    {
        if (_context.Database.IsSqlite())
        {
            return await _context.LlmRequests
                .FromSqlInterpolated($"SELECT * FROM LlmRequests WHERE UserId = {userId} AND Status = {(int)status} ORDER BY CreatedAt DESC")
                .AsNoTracking()
                .Include(lr => lr.Board)
                .ToListAsync(cancellationToken);
        }

        return await _context.LlmRequests
            .AsNoTracking()
            .Include(lr => lr.Board)
            .Where(lr => lr.UserId == userId && lr.Status == status)
            .OrderByDescending(lr => lr.CreatedAt)
            .ToListAsync(cancellationToken);
    }

[Chris0Jeky]

Fixed: added .AsNoTracking() to both SQLite and LINQ branches of GetByUserAndStatusAsync. Also added .Include(lr => lr.User) for consistency with GetByStatusAsync.

[Chris0Jeky]

Adversarial Review (Independent Pass)

Verdict: APPROVE (with one NOTE worth tracking)

Findings

1. All LlmQueueController endpoints scoped — PASS with one pre-existing note

The three list-facing endpoints (POST /, GET /user, GET /status/{status}, GET /stats) all extract userId via TryGetCurrentUserId and pass it to scoped service/repository calls. POST /{id}/cancel also scopes by user ID.

POST /process-next intentionally does NOT scope by user — it advances the next globally-pending item regardless of caller identity. The controller class has [Authorize] so it still requires a valid token; it just does not filter by caller. This matches the PR description ("ProcessNextRequestAsync is intentionally left unscoped — it is a system/worker operation") and matches the background worker behaviour. This is a pre-existing design that was not changed by this PR. NOTE: the endpoint is exposed to any authenticated user, not just admins — this pre-dates this PR and is out of scope, but worth a follow-up.

2. UserId claim extraction safe on missing claim — PASS

TryGetCurrentUserId in AuthenticatedControllerBase returns false and sets errorResult to 401 Unauthorized when UserContext.IsAuthenticated is false or UserId is null/whitespace, and again when Guid.TryParse fails. Both new call-sites return errorResult! immediately. The null-forgiving operator ! is safe here because errorResult is always assigned when the method returns false.

3. Repository method used correctly everywhere — PASS with NOTE

GetByUserAndStatusAsync is used in both GetQueueByStatusAsync and GetQueueStatsAsync. The old GetByStatusAsync is still present and still used in three legitimate system-level contexts:

• LlmQueueToProposalWorker.ProcessBatchAsync — worker needs all users' items (correct)
• HealthController — queue-depth health check, no per-user data exposed (correct)
• OpsCliService.ExecuteQueuePendingAsync — role-gated to admin; intentionally unscoped system view (correct)
• LlmQueueService.ProcessNextRequestAsync — system-level operation (correct)

No unscoped call survives in a user-facing code path.

NOTE (cosmetic): GetByUserAndStatusAsync in the SQLite branch does not include lr.User (the navigation property), unlike GetByStatusAsync. This is not a bug — MapToDto only uses scalar fields from LlmRequest and never touches .User. But the inconsistency could cause a subtle NullReferenceException if callers are ever added that access request.User on the returned objects. Consider adding Include(lr => lr.User) for consistency.

4. Tests use distinct users and non-vacuous assertions — PASS

GetByStatus_ShouldOnlyReturnCurrentUserRequests: User A and User B are separate HttpClient instances, separately authenticated with unique stems (llm-status-isolation-userA / llm-status-isolation-userB). User A creates a known item; User B's result list is asserted to NOT contain any item with UserId == userA.UserId. The test is non-vacuous: the created item is confirmed 200 OK before the cross-user query.

GetQueueStats_ShouldOnlyCountCurrentUserRequests: User B's PendingCount is captured as a baseline before User A creates an item, then asserted to equal the same value after. This correctly detects leakage even if the baseline is non-zero (e.g., from parallel test runs).

Both tests use distinct HttpClient instances with independent auth cookies/tokens. The assertions are meaningful.

5. Worker path intentionally unscoped and justified — PASS

LlmQueueToProposalWorker calls unitOfWork.LlmQueue.GetByStatusAsync (the unscoped method) directly. This is correct: a background worker must process all users' queue items fairly. The PR description explicitly calls this out. There is no data leakage through side channels — the worker processes items and writes back to the same rows; it does not expose user A's data to user B.

6. No compile errors introduced — PASS

dotnet build backend/Taskdeck.sln -c Release → Build succeeded. 0 Warnings, 0 Errors.

7. Tests pass — PASS

dotnet test filtered to LlmQueue → 34 tests (16 Api.Tests + 18 Application.Tests), 0 failed.

Categories checked

• All LlmQueueController endpoints scoped: PASS — all user-facing list endpoints now gate on userId; process-next intentionally unscoped system op
• UserId claim extraction safe on missing claim: PASS — returns 401 on missing/invalid claim
• Repository method used correctly everywhere: PASS — unscoped method retained only in legitimate system paths; new scoped method used in all user-facing paths
• Tests use distinct users and non-vacuous assertions: PASS — separate HttpClient instances, baseline-delta pattern for stats, positive assertion on item identity for status
• Worker path intentionally unscoped and justified: PASS — uses GetByStatusAsync directly, no service layer, no user data exposure
• No compile errors introduced: PASS
• Tests pass: PASS — 34/34

One actionable note (not a blocker)

The GetByUserAndStatusAsync SQLite and LINQ branches both omit Include(lr => lr.User) that GetByStatusAsync includes. MapToDto doesn't access .User so there's no current bug, but the navigation property inconsistency is a trap for future callers. Low priority, safe to address in a follow-up.

[Chris0Jeky]

Gemini Feedback Addressed

Both inline comments from Gemini have been resolved:

[HIGH] GetQueueStatsAsync — single GROUP BY query
Replaced 4 separate GetByUserAndStatusAsync calls with a new GetStatusCountsByUserAsync repository method that fetches all status counts in a single GROUP BY query. DTO is built from the returned dictionary using .GetValueOrDefault(status, 0).

[MEDIUM] AsNoTracking on GetByUserAndStatusAsync
Added .AsNoTracking() to both SQLite and LINQ branches. Also added .Include(lr => lr.User) to close the navigation-property consistency gap noted in the prior adversarial review.

Build: clean. LlmQueue tests: all pass.