LLM-02: Expand board context with card IDs and structured reference

[Chris0Jeky]

Summary

• Expand BoardContextBuilder to include short card IDs (first 8 hex chars of GUID) in bracket notation, enabling the LLM to generate move card <id> style instructions
• Restructure context format: column flow line (A -> B -> C), per-column card groupings with Cards in "Column": headings, card-level label annotations
• Increase context budget from 2000 to 4000 characters to accommodate the richer card metadata
• Skip empty columns in card listing section to conserve budget
• Add FormatShortId helper with deterministic output (no-hyphen hex prefix)

Closes #617

Test plan

• All 15 BoardContextBuilder tests pass (including 4 new tests)
• Full backend test suite passes (530+ tests, 0 failures)
• New tests cover: short ID formatting, card-level labels in output, empty column skipping, budget constant verification
• Existing truncation and budget tests still pass with the increased limit

[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

What changed

1. BoardContextBuilder.cs — new structured format with card IDs (first 8 hex chars), column flow line, per-column card groupings with card-level labels, budget increased from 2000 to 4000 chars.
2. BoardContextBuilderTests.cs — updated existing tests for new format, added 4 new tests (FormatShortId, card labels, empty column skipping, budget constant).

Review findings

Correctness:

• FormatShortId uses Guid.ToString("N") which produces 32 hex chars without hyphens, then takes first 8. This is deterministic and stable for any valid GUID.
• The label lookup via Dictionary<Guid, string> correctly maps card-level CardLabel join entities to their label names.
• Empty columns are skipped in card listings but still appear in the flow line — this is the right behavior since the column structure matters for the LLM even when columns are empty.
• Truncation logic is unchanged and still works correctly with the increased budget.

Potential concerns (all minor, no action needed):

• CardLabels navigation property: if EF Core does not eagerly load CardLabels when fetching cards via GetByColumnIdAsync, the label annotation will silently produce no labels rather than failing. This is acceptable behavior — the context degrades gracefully. Eager loading can be added to the repository query later if needed.
• The → arrow character in the column flow line is UTF-8 multi-byte. This is fine for the LLM prompt context but worth noting in case a future consumer expects ASCII-only output.

Test coverage:

• All happy paths covered: board name, column flow, card IDs, card labels, empty columns, budget enforcement, truncation.
• Edge cases: null board returns null, no columns omits section, no labels omits section.
• Full backend suite (530+ tests) passes with 0 failures.

Verdict: Ship-ready. No blocking issues found.

[gemini-code-assist]

Code Review

This pull request enhances the BoardContextBuilder by increasing the character budget for LLM prompts and improving the context format to include short card IDs and labels. A performance issue was identified where cards are fetched individually for each column, creating an N+1 query pattern; it is recommended to fetch all cards for the board in a single query instead.

[gemini-code-assist]

The current implementation fetches cards for each column individually within the loop, resulting in an N+1 query pattern. This can cause significant performance degradation as the number of columns increases. It is more efficient to fetch all cards for the board in a single query and group them by column in memory.

        if (columns.Count > 0)
        {
            sb.Append("Columns: ").AppendLine(
                string.Join(" → ", columns.Select(c => c.Name)));

            var cardsByColumn = (await _unitOfWork.Cards.GetByBoardIdAsync(boardId, ct))
                .GroupBy(c => c.ColumnId)
                .ToDictionary(
                    g => g.Key,
                    g => g.OrderByDescending(c => c.UpdatedAt).Take(MaxCardsPerColumn).ToList());

            foreach (var column in columns)
            {
                if (!cardsByColumn.TryGetValue(column.Id, out var columnCards) || columnCards.Count == 0)
                    continue;

                sb.Append("Cards in \"").Append(column.Name).AppendLine("\":");

[Chris0Jeky]

Adversarial Review of PR #638

Overall this is a clean, well-scoped change. The new format is an improvement for LLM instruction parsing. I found no critical issues but several items worth addressing before merge.

---

1. Stale XML doc comment (Low)

File: BoardContextBuilder.cs, lines 7-10 (after patch)

The class-level XML <summary> was updated but still says:

"card IDs and titles per column (with labels), and board-level label names"

However the old comment fragment "The context includes the board name, column names and positions, recent card titles per column, and label names" was only partially updated in the diff. Specifically, the original said "recent card titles" which is now replaced, but the new comment does not mention that cards are ordered by UpdatedAt descending (i.e., "recent" semantics are preserved but undocumented). This is cosmetic but worth a one-line fix for contributor clarity.

Severity: Low

---

2. Budget check is only inside the inner card loop -- column-level overflow continues iterating (Medium)

File: BoardContextBuilder.cs, new lines ~73-88

The truncation if (sb.Length >= MaxContextCharacters) break; only exits the inner foreach (var card in columnCards) loop. After breaking from the card loop, execution continues to the next column in the outer loop, which:

1. Issues another GetByColumnIdAsync DB call (wasted I/O)
2. Materializes the result with .ToList()
3. Only then checks if (columnCards.Count == 0) continue;

There is no budget check after the inner card loop breaks. The old code had a second if (sb.Length >= MaxContextCharacters) break; after the inner foreach that broke out of the outer column loop too. The diff removes this outer break.

Looking at the diff more carefully: the old code had:

foreach (var card in columnCards)
{
    sb.Append("    * ").AppendLine(card.Title);
    if (sb.Length >= MaxContextCharacters)
        break;
}
if (sb.Length >= MaxContextCharacters)
    break;

The new code only has the inner break. The outer break was dropped. This means after hitting the budget mid-column, the builder will continue issuing DB queries for every remaining column, doing wasted work. On a board with 20 columns this is 19 unnecessary round-trips.

Severity: Medium (performance regression, not correctness -- the final truncation marker still enforces the budget on the string itself)

---

3. No test for the truncation boundary with the new richer format (Medium)

File: BoardContextBuilderTests.cs

The existing BuildContextAsync_RespectsTokenBudget test creates 20 columns x 10 cards with long titles, which will exceed the budget. However, this test was written for the old 2000-char budget. With the new 4000-char budget and the same test data, the test still passes but exercises a different truncation point. There is no test that verifies:

• The ...(truncated) marker appears at the right position with the new format (card IDs + labels make lines longer).
• The outer-loop early-exit (or lack thereof, per finding Add testing readiness review and action plan #2) is exercised.
• A board with many labeled cards triggers truncation at the card-label boundary (not mid-label-name, which could produce garbled output like [Bu...(truncated)).

The new BuildContextAsync_BudgetIs4000 test only asserts the constant value, not actual truncation behavior.

Severity: Medium (test gap)

---

4. Short ID collision risk is undocumented (Low)

File: BoardContextBuilder.cs, FormatShortId method

8 hex chars = 32 bits of entropy. For a board with ~100 cards, the birthday-problem collision probability is negligible (~0.0001%). However, this is being surfaced in an LLM prompt where the model may use the short ID to reference cards in instructions. If the LLM generates move card abcdef12 to Done and two cards share that prefix, the downstream parser would need to handle ambiguity.

This is not a bug today (collision is astronomically unlikely at board scale), but the design decision should be documented -- either in the code comment or an ADR note -- so future contributors don't reduce ShortIdLength to 4 without understanding the tradeoff.

Severity: Low (design documentation gap)

---

5. Column flow line (Columns: A -> B -> C) is emitted even when all columns are empty (Low)

File: BoardContextBuilder.cs, new lines ~53-55

The Columns: flow line is emitted unconditionally when columns.Count > 0, but then empty columns are skipped in the card listing. This means for a board with 3 empty columns, the output includes:

Columns: To Do -> In Progress -> Done

...but no card sections. This is actually correct behavior (showing workflow structure is useful even without cards), but it's worth noting that the test BuildContextAsync_SkipsEmptyColumns only asserts that Cards in "Empty Column" is absent -- it does not assert that the column still appears in the flow line, which would be a useful positive assertion.

Severity: Low (test could be stronger)

---

6. GetByColumnIdAsync returns ALL cards, ordering/limiting happens in memory (Informational)

File: BoardContextBuilder.cs, lines ~68-71 (new code)

The comment in the old code said "Fetch cards per column from DB with limit applied at the query level to avoid loading all board cards into memory." This comment was removed. The actual behavior hasn't changed -- GetByColumnIdAsync returns all cards for the column (no server-side limit), and .OrderByDescending(c => c.UpdatedAt).Take(MaxCardsPerColumn) applies in memory after full materialization.

For boards with many cards per column (100+), this loads all cards into memory just to take 5. This is a pre-existing issue, not introduced by this PR, but worth noting since the old misleading comment was removed -- the new code is actually more honest about what's happening.

Severity: Informational (pre-existing, not introduced by this PR)

---

Summary

#	Finding	Severity
2	Outer budget-break removed -- wasted DB queries after budget hit	Medium
3	No test for truncation boundary with new richer format	Medium
1	Stale XML doc (minor)	Low
4	Short ID collision risk undocumented	Low
5	Empty-column flow line test could assert positive case	Low
6	All cards loaded in memory per column (pre-existing)	Informational

Recommendation: Fix #2 (restore the outer break after the inner card loop) before merging. The rest are low-severity improvements that could be follow-ups.