fix(llc): three review findings I merged past on #15920 (#15905)

autobot-backend/llc/services/backlog.py

@@ -65,10 +65,16 @@ async def list_backlog(
         count_q = select(func.count()).select_from(q.subquery())
         total = (await session.execute(count_q)).scalar_one()
 
-        # Order by explicit backlog_position first (NULLS LAST — items that have
-        # never been reordered keep their natural priority/age ordering), then
-        # fall back to priority rank, then creation date.  This makes
-        # bulk_reorder's writes immediately observable in list responses (H3).
+        # Order by explicit backlog_position, then priority rank, then creation
+        # date. This makes bulk_reorder's writes observable in list responses (H3).
+        #
+        # #15963: the `nulls_last` in `backlog_order()` is DEAD, and the sentence
+        # that used to be here claimed the opposite. The column is
+        # `nullable=False` with `server_default="0"` and `bulk_reorder` assigns
+        # `0..n-1`, so no row is ever NULL: items never reordered sit at 0 and
+        # sort AHEAD of anything reordered to position >= 1. #15964 corrected
+        # half of this comment and left the rest — the same neighbour miss the
+        # correction was about.
         q = q.order_by(*backlog_order()).limit(limit).offset(offset)
         rows = (await session.execute(q)).scalars().all()
         return rows, total

autobot-backend/llc/services/work_item_queue.py

@@ -28,8 +28,9 @@
 import uuid
 from typing import Any, List, Optional
 
-from sqlalchemy import case, nulls_last, select
+from sqlalchemy import and_, case, nulls_last, or_, select
 from sqlalchemy.ext.asyncio import AsyncSession
+from sqlalchemy.sql.elements import ColumnElement
 
 from ..models.enums import WorkItemPriority, WorkItemStatus
 from ..models.work_item import LLCWorkItem
@@ -55,14 +56,48 @@
 def backlog_order() -> List[Any]:
     """The backlog's ordering, as `order_by` arguments.
 
-    Explicit `backlog_position` first (NULLS LAST, so items never reordered keep
-    their natural priority/age ordering), then priority rank, then age. This is
-    what makes a `bulk_reorder` write immediately observable to both the list
-    view and the next agent to ask for work.
+    Explicit `backlog_position` first, then priority rank, then age. This is what
+    makes a `bulk_reorder` write immediately observable to both the list view and
+    the next agent to ask for work.
+
+    **The `nulls_last` is currently dead, and the comment it replaces was wrong.**
+    That comment said NULLS LAST lets items never reordered keep their natural
+    priority/age ordering. `backlog_position` is `nullable=False` with
+    `server_default="0"` and `bulk_reorder` assigns `0..n-1`, so no row is ever
+    NULL: un-reordered items sit at 0 and therefore sort AHEAD of anything
+    reordered to position >= 1. `nulls_last` is retained because it costs nothing
+    and becomes correct the moment the column takes a sentinel, but it is not
+    doing the work the old wording claimed. Tracked as #15963 -- the fix is a
+    sentinel plus a data migration, and existing `0` rows are ambiguous between
+    "untouched" and "reordered to first", which no migration can separate.
     """
     return [nulls_last(LLCWorkItem.backlog_position.asc()), PRIORITY_RANK, LLCWorkItem.created_at.asc()]
 
 
+def claimable_by(agent_uuid: uuid.UUID) -> ColumnElement[bool]:
+    """Assignment predicate: unassigned, or already this agent's.
+
+    "Unassigned" requires BOTH assignee columns to be null. Testing only
+    `assignee_agent_id` let an agent claim an item assigned to a USER
+    (CWE-863) — and `checkout` writes `assignee_type = agent` for whatever it
+    claims, so the row came out naming an agent and a user at once, with the
+    type agreeing with only one of them. `checkout` now also clears
+    `assignee_user_id`, which keeps a row consistent; this predicate is what
+    stops the claim happening at all.
+
+    Split out of the `checkout_next` SELECT so the rule can be compiled and
+    asserted on directly — the behavioural path needs Postgres and skips
+    without it, and a security regression test that skips is not a test.
+    """
+    return or_(
+        and_(
+            LLCWorkItem.assignee_agent_id.is_(None),
+            LLCWorkItem.assignee_user_id.is_(None),
+        ),
+        LLCWorkItem.assignee_agent_id == agent_uuid,
+    )
+
+
 async def checkout_next(
     session: AsyncSession,
     service: Any,
@@ -98,13 +133,17 @@ async def checkout_next(
             LLCWorkItem.company_id == uuid.UUID(company_id),
             LLCWorkItem.status == WorkItemStatus.READY.value,
             LLCWorkItem.checkout_run_id.is_(None),
+            # In SQL, not in Python after the fact. The LIMIT must apply to
+            # ELIGIBLE rows: filtering afterwards meant ten items assigned to
+            # other agents returned "no work" while eligible items sat below
+            # them, and the bound silently became "how many of the top ten are
+            # mine" rather than "how many claims will I attempt".
+            claimable_by(uuid.UUID(agent_id)),
         )
         .order_by(*backlog_order())
         .limit(CHECKOUT_CANDIDATES)
     )
     for item in (await session.execute(eligible)).scalars().all():
-        if item.assignee_agent_id is not None and str(item.assignee_agent_id) != agent_id:
-            continue
         try:
             return await service.checkout(
                 session,

autobot-backend/llc/services/work_item_service.py

@@ -510,10 +510,10 @@ async def checkout(
         item.checkout_run_id = run_id or str(uuid.uuid4())
         item.checkout_locked_at = datetime.now(timezone.utc)
         item.assignee_agent_id = uuid.UUID(agent_id)
+        # Single-assignee invariant (#10532): `update()` enforced it, `checkout` did not (#15964).
+        item.assignee_user_id = None
         item.assignee_type = AssigneeType.AGENT.value
-        # GH#9532 — persist intent for audit trail.  Clearing prior intent when
-        # work_intent is absent is deliberate: stale intent must not survive a
-        # new checkout.
+        # GH#9532 — persist intent for audit; clearing it when work_intent is absent is deliberate.
         item.checkout_intent = work_intent
         item.version += 1
         if item.status in (WorkItemStatus.BACKLOG, WorkItemStatus.READY):

autobot-backend/llc/tests/test_agent_stub_routes_15905.py

@@ -254,6 +254,11 @@ async def test_the_comment_route_stores_a_readable_comment(session):  # noqa: AN
 
     company = str(uuid.uuid4())
     item = await _seed_item(session, company)
+    # The node must exist or `agent_node_uuid` returns None and the authorship
+    # assertion below is vacuous — which is what this test did before #15920's
+    # review caught it. A test that never seeds the row cannot tell "author
+    # resolved" from "author silently dropped".
+    node_id = await _seed_agent_node(session, company, "agent-c")
 
     with patch("user_management.database.get_async_session_factory", new=_factory_yielding(session)):
         result = await agent_api.post_comment(
@@ -270,6 +275,9 @@ async def test_the_comment_route_stores_a_readable_comment(session):  # noqa: AN
     assert len(stored) == 1, "the route reported the comment recorded and nothing was stored"
     assert stored[0].body == "the agent said this"
     assert str(stored[0].id) == result["comment_id"]
+    assert (
+        stored[0].author_agent_id == node_id
+    ), "the comment was stored without its author; `agent_node_uuid` resolved to None"
 
 
 async def test_the_comment_route_refuses_another_companys_item(session):  # noqa: ANN001
@@ -516,3 +524,55 @@ async def test_a_malformed_work_item_id_is_422_on_both_routes(session):  # noqa:
             _request(agent, company),
         )
     assert via_heartbeat.value.status_code == 422
+
+
+async def test_the_next_route_looks_past_items_assigned_to_other_agents(session):  # noqa: ANN001
+    """#15920 review: the assignee filter must be IN the query, not after it.
+
+    `checkout_next` LIMITs to `CHECKOUT_CANDIDATES` and used to discard
+    other-agent items afterwards — so a backlog whose top `CHECKOUT_CANDIDATES`
+    items belong to someone else returned "no eligible work" while eligible
+    items sat directly below them. The bound silently meant "how many of the top
+    ten are mine" rather than "how many claims will I attempt".
+    """
+    from unittest.mock import patch
+
+    from llc.api import agent_api
+    from llc.services.work_item_queue import CHECKOUT_CANDIDATES
+
+    company = str(uuid.uuid4())
+    await _seed_agent_node(session, company, "agent-patient")
+    other = await _seed_agent_node(session, company, "agent-other")
+
+    # Fill every candidate slot with items assigned elsewhere, then one for us
+    # below them.
+    for n in range(CHECKOUT_CANDIDATES):
+        taken = await _seed_item(session, company, title=f"theirs-{n}", position=n)
+        taken.assignee_agent_id = other
+        await session.commit()
+    mine = await _seed_item(session, company, title="mine", position=CHECKOUT_CANDIDATES + 1)
+
+    with patch("user_management.database.get_async_session_factory", new=_factory_yielding(session)):
+        result = await agent_api.get_next_work_item(_request("agent-patient", company))
+
+    assert result["checked_out"] is True, "returned no work while an eligible item sat below the candidate window"
+    assert result["work_item"]["id"] == str(mine.id)
+
+
+async def test_the_next_route_still_takes_an_item_assigned_to_this_agent(session):  # noqa: ANN001
+    """The contrast: the SQL predicate must not exclude our OWN assignments."""
+    from unittest.mock import patch
+
+    from llc.api import agent_api
+
+    company = str(uuid.uuid4())
+    node_id = await _seed_agent_node(session, company, "agent-owner")
+    item = await _seed_item(session, company, title="already mine")
+    item.assignee_agent_id = node_id
+    await session.commit()
+
+    with patch("user_management.database.get_async_session_factory", new=_factory_yielding(session)):
+        result = await agent_api.get_next_work_item(_request("agent-owner", company))
+
+    assert result["checked_out"] is True
+    assert result["work_item"]["id"] == str(item.id)

autobot-backend/tests/unit/llc/test_checkout_excludes_user_assigned_15964.py

@@ -0,0 +1,129 @@
+# Copyright 2025-2026 mrveiss
+# SPDX-License-Identifier: Apache-2.0
+# AutoBot - AI-Powered Automation Platform
+# Author: mrveiss
+"""An agent must not claim a work item assigned to a user (#15964, CWE-863).
+
+`checkout_next`'s eligibility predicate originally tested only
+`assignee_agent_id IS NULL` for "unassigned". An item assigned to a *user* has
+that column null, so it matched, and `checkout` then wrote
+`assignee_type = agent` over it while leaving `assignee_user_id` populated —
+a row naming both an agent and a user, its type agreeing with one of them.
+
+These assertions read the predicate's expression TREE rather than the compiled
+SQL string. A string test passes on `... IS NULL` appearing anywhere in the
+statement, including inside the unrelated `checkout_run_id` filter; the shape
+is what carries the rule.
+
+The behavioural path needs a real Postgres (postgresql.UUID/JSONB do not
+compile on SQLite) and would skip everywhere else. A security regression test
+that skips reports clean while proving nothing, so the rule is pinned here,
+unconditionally, and the writer half is pinned by AST below.
+"""
+
+from __future__ import annotations
+
+import ast
+import uuid
+from pathlib import Path
+
+from sqlalchemy import or_
+
+from llc.models.work_item import LLCWorkItem
+from llc.services.work_item_queue import claimable_by
+
+BACKEND_ROOT = Path(__file__).resolve().parents[3]
+
+
+def _columns_compared_to_null(clause) -> set[str]:
+    """Names of columns this clause tree tests with `IS NULL`."""
+    found: set[str] = set()
+    stack = [clause]
+    while stack:
+        node = stack.pop()
+        clauses = getattr(node, "clauses", None)
+        if clauses is not None:
+            stack.extend(clauses)
+            continue
+        if getattr(node, "operator", None) is not None or hasattr(node, "left"):
+            left, right = getattr(node, "left", None), getattr(node, "right", None)
+            if left is not None and right is not None:
+                is_null = right.__class__.__name__ == "Null" or getattr(right, "value", ...) is None
+                if is_null and hasattr(left, "key"):
+                    found.add(left.key)
+    return found
+
+
+def test_unassigned_branch_requires_both_assignee_columns_null() -> None:
+    """The "unassigned" branch must test the user column as well as the agent one."""
+    predicate = claimable_by(uuid.uuid4())
+
+    branches = list(predicate.clauses)
+    assert len(branches) == 2, f"expected an OR of 2 branches, got {len(branches)}"
+
+    unassigned = next(
+        (b for b in branches if _columns_compared_to_null(b)),
+        None,
+    )
+    assert unassigned is not None, "no branch tests any column for NULL"
+
+    null_tested = _columns_compared_to_null(unassigned)
+    assert null_tested == {"assignee_agent_id", "assignee_user_id"}, (
+        "The unassigned branch of the checkout predicate must require BOTH "
+        f"assignee columns to be NULL; it tests {sorted(null_tested)}. Testing "
+        "only assignee_agent_id lets an agent claim a user-assigned item "
+        "(#15964, CWE-863)."
+    )
+
+
+def test_the_rule_rejects_the_predicate_this_issue_was_filed_about() -> None:
+    """Contrast pair: the OLD, agent-only predicate must NOT satisfy the rule.
+
+    Without this, the assertion above is indistinguishable from one that
+    reports "both columns" for any shape it is handed — the test would pass on
+    the vulnerable predicate just as happily as on the fixed one.
+    """
+    vulnerable = or_(
+        LLCWorkItem.assignee_agent_id.is_(None),  # the pre-#15964 predicate
+        LLCWorkItem.assignee_agent_id == uuid.uuid4(),
+    )
+
+    branches = list(vulnerable.clauses)
+    unassigned = next(b for b in branches if _columns_compared_to_null(b))
+    null_tested = _columns_compared_to_null(unassigned)
+
+    assert null_tested == {"assignee_agent_id"}, (
+        "The reader must see the old predicate as testing ONLY the agent "
+        f"column; it reported {sorted(null_tested)}. If it cannot tell the two "
+        "predicates apart, the test above proves nothing about the fixed one."
+    )
+
+
+def test_checkout_clears_the_user_assignee_when_an_agent_claims() -> None:
+    """`checkout` must not leave `assignee_user_id` set on an agent-claimed row.
+
+    Read from the source: the write sits inside an async method that needs a
+    session and a live row. What matters for the invariant is that the
+    assignment exists at all, next to the `assignee_agent_id` write.
+    """
+    source = (BACKEND_ROOT / "llc" / "services" / "work_item_service.py").read_text(encoding="utf-8")
+    tree = ast.parse(source)
+
+    checkout = next(
+        (n for n in ast.walk(tree) if isinstance(n, ast.AsyncFunctionDef) and n.name == "checkout"),
+        None,
+    )
+    assert checkout is not None, "checkout() not found — this test's anchor moved"
+
+    cleared = any(
+        isinstance(node, ast.Assign)
+        and isinstance(node.value, ast.Constant)
+        and node.value.value is None
+        and any(isinstance(t, ast.Attribute) and t.attr == "assignee_user_id" for t in node.targets)
+        for node in ast.walk(checkout)
+    )
+    assert cleared, (
+        "checkout() sets assignee_agent_id and assignee_type = agent but never "
+        "clears assignee_user_id, so an agent-claimed row keeps naming a user "
+        "assignee too (#15964)."
+    )