diff --git a/docs/decisions/retrieval-eval.md b/docs/decisions/retrieval-eval.md new file mode 100644 index 000000000..82f184845 --- /dev/null +++ b/docs/decisions/retrieval-eval.md @@ -0,0 +1,182 @@ +# Retrieval-eval question bank (decision-lifecycle corpus, #521) + +This bank measures **correct active-record selection + citation**, not mere semantic proximity. +The migrated decision corpus (`docs/decisions.md` + `docs/decisions/*.md`, catalog at +`docs/decisions/README.md`, archive at `docs/decisions/archive/`) intentionally keeps a superseded +or retired record's prose *verbatim* next to a live `superseded-by`/`supersedes` pointer. A retrieval +approach that matches on wording alone can walk straight past that pointer into the archived record +and return a decision this project no longer follows. For each question below, the only correct +answer is the cited active `key` (status: `active`, in a file `active_files()` globs) — landing on +an archived/superseded key, or missing a key that already answers the question (and thus proposing a +reimplementation), is scored a **miss**, regardless of how relevant the returned text reads. + +This bank is run as the **Task 10 cold-agent retrieval sim**: a fresh agent is given the catalog +(`docs/decisions/README.md`) plus the active `docs/decisions/*.md` wing and asked each question with +no other context, then graded on whether it names the expected `key` and cites the right file. + +Verification: every `key` cited below was confirmed present and `status: active` via +`PYTHONPATH=. python3 -c "import scripts.decisions_lib as dl; [print(r.key, r.status) for f in dl.active_files() for r in dl.parse_file(f) if r.key]"` +against this worktree's corpus on 2026-07-21. + +--- + +## 1. Paraphrased task → decision discovery + +**Q1.** "I'm adding a new REST endpoint for a feature that touches three tables — do I need to stand +up a service/business-logic layer, or can the controller call MediatR directly?" +- **Expected key:** `api.mediatr-passthrough` +- **File:** `docs/decisions.md` +- **Why:** The rule is literally "thin controllers over existing MediatR handlers, no new + service/business-logic layer" — answer is NO, don't add one. A naive search might instead surface + `mcp.server-foundation` (also MediatR-adjacent, wrong layer) or nothing at all if it only matches + on "REST endpoint" rather than the layering question. + +**Q2.** "Should the playout-build-finished notification go out over a websocket/SignalR push channel +so the SPA doesn't have to poll?" +- **Expected key:** `api.async-op-contract` +- **File:** `docs/decisions.md` +- **Why:** The active rule explicitly rejects a live push channel in favor of an `isLocked` + HTTP-observable flag on list/detail GETs as the substitute. A naive search keying on "poll" alone + might return `spa.playback-troubleshoot-poll` (a different, narrower polling decision for the + troubleshoot screen) instead of the general async-op contract that actually governs new + queue-triggering endpoints. + +## 2. Exact code/path lookup + +**Q3.** "Where do new API response DTOs live, and what nullable pragma convention do they follow?" +- **Expected key:** `api.response-dtos` +- **File:** `docs/decisions.md` +- **Why:** Direct hit — rule gives the exact path pattern + (`ErsatzTV.Core/Api//*ResponseModel.cs`) and the file-scoped `#nullable enable` convention. + No plausible superseded alternative exists for this key. + +**Q4.** "Where does the `EntityLocker` implementation live, and how does it avoid a torn +check-then-set race on its lock flags?" +- **Expected key:** `locking.entitylocker-atomic-flags` +- **File:** `docs/decisions.md` +- **Why:** Rule + Mechanics cite `ErsatzTV.Infrastructure/Locking/EntityLocker.cs` and the + `Interlocked.CompareExchange`-guarded atomic-flag fix directly; no other record touches this file. + +## 3. Active-vs-superseded (selecting the archived record here is a FAILURE) + +**Q5** *(verbatim, coordinated with server-management#642).* "Is #390's small-lane CI move current?" +- **Expected answer:** **No** — superseded by `ci.runner-placement`. +- **Expected key:** `ci.runner-placement` +- **File:** `docs/decisions.md` +- **Why:** #390 moved `docker build` jobs onto the `small` runner lane to dodge queue time; #406 + reversed that (worst-case memory, not runtime, was the real constraint) and the durable rule now + lives under `ci.runner-placement` (and its sibling `ci.small-lane-git-only`, which defines + `small` by job *kind*, not usual runtime). #390 itself was never given its own `##` record (it's + prose-only, referenced inside `ci.runner-placement`'s own Signals line) — answering as if #390's + move still holds, or citing #390 as a standalone current decision, is the failure mode this + question targets. + +**Q6.** "Is `docs/decisions.md` still kept append-only by a line-deletion-diff CI/hook guard?" +- **Expected answer:** **No** — that mechanism is superseded. +- **Expected key:** `docs.decision-lifecycle` +- **File:** `docs/decisions.md` +- **Superseded record a naive search might return instead:** `docs.append-only-guard` + (`docs/decisions/archive/release-ci-governance.md`, `status: superseded`, + `superseded-by: docs.decision-lifecycle@2026-07-21`) +- **Why:** `docs.append-only-guard`'s prose (numstat-deleted-count hook + CI job) still reads as a + perfectly good, on-topic answer to "is decisions.md append-only" — that's exactly the trap: the + *mechanism* it describes was replaced by the lifecycle-schema validator + (`scripts/decisions_validate.py`) that this migration introduced. Selecting the archived record + instead of following its `superseded-by` pointer to `docs.decision-lifecycle` is the scored + failure. + +**Q6b** *(deferred — flag, do not score yet).* "Does volatile session/queue state still live in the +pinned Gitea tracker issue #237 handoff pattern, or has that moved to a parallel-orientation +mechanism?" +- **Current state in this corpus snapshot:** `docs.queue-state-gitea-tracker` is still `status: + active` (`docs/decisions.md`) — Task 8's `startup.parallel-orientation` record had not landed in + this worktree as of this bank's authoring (2026-07-21). Once Task 8 merges and supersedes it, this + becomes a third scored active-vs-superseded pair (expected key: `startup.parallel-orientation`; + superseded key: `docs.queue-state-gitea-tracker`). Left in the bank as a placeholder per the Task 9 + brief; re-verify both keys' `status` before scoring it. + +## 4. Retired feature + +No `status: retired` record exists yet anywhere in the migrated corpus (confirmed via +`grep -rn "status: retired" docs/decisions.md docs/decisions/*.md docs/decisions/archive/*.md` — +zero hits). Using the one available superseded record as the closest analog instead: + +**Q7.** "Has the Husky `commit-msg` decisions-guard hook (the one that blocks any line-deletion diff +to `decisions.md`) been removed now that decisions carry a lifecycle schema?" +- **Expected answer:** Superseded, not simply "removed" — the append-only-by-diff mechanism is + replaced by `scripts/decisions_validate.py`'s lifecycle-field checks; the sibling H3 + root-screenshot guard from the same original record was split out and is **still active** today. +- **Expected key:** `docs.decision-lifecycle` (supersedes `docs.append-only-guard`); sibling active + key `ci.root-screenshot-guard` (`docs/decisions/release-ci-governance.md`) must NOT be reported as + superseded — it's a distinct, still-live record split from the same legacy heading. +- **Why:** Tests that an agent doesn't over-generalize "the old #303 H9/H3 heading was archived" into + wrongly retiring the H3 half too — the migration-map explicitly documents the split. + +## 5. Rationale / rejected-alternative + +**Q8.** "Why doesn't `EntityLocker` use owner tokens or lease objects instead of plain unlock calls?" +- **Expected key:** `locking.entitylocker-atomic-flags` +- **File:** `docs/decisions.md` +- **Why:** The record's body explicitly documents the single-owner-release discipline as the chosen + design and that `Unlock*` on an already-unlocked slot returns `false` + logs a Warning rather than + throwing — the rejected alternative (owner tokens/leases) is named in the Rule line itself. + +**Q9.** "Why did the project reject a CI build-once shared-compile-artifact approach?" +- **Expected key:** `ci.build-once-rejected` +- **File:** `docs/decisions.md` +- **Why:** Rule states it plainly: measured and rejected for a 40-85% wall-clock regression, keeping + the #420 cross-run tree-identity skip instead. A search for "CI build speed" alone might surface + `ci.docs-only-skip-steps` or `ci.peak-anon-measurement` (both real, both wrong for "why was + build-once rejected"). + +## 6. Convention already implemented — do NOT reimplement + +**Q10.** "We need seasonal/holiday scheduling (different programming around Christmas, say) — should +I design a new date-conditional scheduling feature?" +- **Expected key:** `sched.seasonal-scheduling-existing` +- **File:** `docs/decisions.md` +- **Why:** Already ships via `IAlternateScheduleItem` (Classic `ProgramScheduleAlternate`, Block + `PlayoutTemplate`) evaluated by `AlternateScheduleSelector.GetScheduleForDate`; #73 was closed as + already-implemented with a docs-only recipe added. Building a new feature here duplicates existing, + shipped functionality — the correct answer is "use alternate schedules," not a design doc. + +**Q11.** "Should I add a way to pad content out to the next clock boundary (e.g. keep a channel's +7:00 PM start exact) per channel?" +- **Expected key:** `sched.clock-padding-existing` +- **File:** `docs/decisions.md` +- **Why:** Already exists via `FillerPreset`'s `FillerMode.Pad` (Classic) and + `pad_to_next`/`pad_until` (Sequential/YAML); #77 was closed as verified+documented, not built new. + Only the one-click per-channel UI toggle is deferred (behind the #388 design-system epic) — the + underlying capability is not missing. + +**Q12.** "Do we need to build a fair-share / weighted rotation mode for multi-collections, or does +`ShuffleInOrder` already cover that?" +- **Expected key:** `sched.weighted-shuffle` +- **File:** `docs/decisions.md` +- **Why:** This one cuts the other way on purpose — `ShuffleInOrder` only anti-clumps (its padding + spacers emit nothing), so it does NOT already cover fair-share; the correct answer is that a new + `PlaybackOrder.WeightedShuffle = 9` order was deliberately added rather than retrofitting + `ShuffleInOrder`. Tests that "already exists, don't rebuild" isn't over-applied to a + superficially-similar existing feature that actually doesn't do the job. + +--- + +## Class coverage summary + +| # | Class | Key(s) | +| - | ----- | ---- | +| Q1 | paraphrased-discovery | `api.mediatr-passthrough` | +| Q2 | paraphrased-discovery | `api.async-op-contract` | +| Q3 | exact-lookup | `api.response-dtos` | +| Q4 | exact-lookup | `locking.entitylocker-atomic-flags` | +| Q5 | active-vs-superseded (verbatim #390) | `ci.runner-placement` | +| Q6 | active-vs-superseded | `docs.decision-lifecycle` (vs archived `docs.append-only-guard`) | +| Q6b | active-vs-superseded (deferred, unscored) | `startup.parallel-orientation` (pending Task 8) vs `docs.queue-state-gitea-tracker` | +| Q7 | retired-feature (no `retired` record exists; superseded used as analog) | `docs.decision-lifecycle` / `ci.root-screenshot-guard` (must stay active) | +| Q8 | rationale/rejected-alternative | `locking.entitylocker-atomic-flags` | +| Q9 | rationale/rejected-alternative | `ci.build-once-rejected` | +| Q10 | convention-already-implemented | `sched.seasonal-scheduling-existing` | +| Q11 | convention-already-implemented | `sched.clock-padding-existing` | +| Q12 | convention-already-implemented (negative control) | `sched.weighted-shuffle` | + +12 scored questions (Q1–Q12, excluding the deferred Q6b placeholder) across all six required classes.