Trevin Chow
68ae74ff4f
Closes #374 (adapted, not 1:1 merged). @hnshah opened PR #374 proposing a docs/adr/ directory for architecture decision records. The intent is right -- the "why is search-quality eval manual?" reasoning drifts out of memory if it isn't written down -- but the docs/adr/ convention doesn't fit alongside the existing docs/solutions/ structure (compound-engineering ce-compound pattern with frontmatter metadata, additive entries, no membership-contract test). This commit adopts hnshah's ADR 002 content (search-quality eval is manual by default) as a docs/solutions/architecture/ entry with the canonical compound-style frontmatter (module, problem_type, applies_when, related_components, tags). Drops the docs/adr/ directory pattern, the README index, and the test_adr_docs.py contract test. ADR 001 (multi-surface packaging) is intentionally not adopted here: it referenced sync.sh as the deploy mechanism, but sync.sh was removed in PR #405 in favor of `npx skills add . -g -y`. The multi-surface packaging story is still real but has moved beyond what the original ADR captured; a fresh "how we ship to multiple harnesses" entry would make sense as a separate doc. Co-authored-by: hnshah <hnshah@users.noreply.github.com>
83 lines
4.9 KiB
Markdown
83 lines
4.9 KiB
Markdown
---
|
|
title: Search-quality eval is manual by default, not a CI gate on every PR
|
|
date: 2026-05-10
|
|
category: docs/solutions/architecture
|
|
module: skills/last30days/scripts/evaluate_search_quality.py
|
|
problem_type: design_decision
|
|
component: ci_policy
|
|
severity: low
|
|
applies_when:
|
|
- a contributor proposes wiring search-quality eval into PR CI
|
|
- a change affects retrieval, ranking, grounding, or synthesis quality and a reviewer asks "why aren't we testing this in CI?"
|
|
- someone is deciding whether a new evaluator-style script belongs in the default CI workflow
|
|
related_components:
|
|
- search_quality_evaluation
|
|
- ci_workflow
|
|
- llm_judging
|
|
tags:
|
|
- ci-policy
|
|
- eval
|
|
- design-decision
|
|
- cost-vs-signal
|
|
- non-determinism
|
|
- manual-gates
|
|
---
|
|
|
|
# Search-quality eval is manual by default, not a CI gate on every PR
|
|
|
|
## Context
|
|
|
|
`skills/last30days/scripts/evaluate_search_quality.py` compares a baseline revision against a candidate revision across a fixed pool of reviewer topics. It produces two flavors of metrics: deterministic overlap (Jaccard, retention) and LLM-judged quality scores. The natural impulse on seeing an evaluator script is to wire it into CI on every PR — "regression catcher, run it automatically." We deliberately don't.
|
|
|
|
Three properties of this particular evaluator make CI-on-every-PR the wrong default:
|
|
|
|
1. **Live API access.** The candidate revision typically needs the engine to actually run, which means real ScrapeCreators calls, real reddit fetches, real YouTube searches. CI runs would either need production credentials or a record/replay fixture set that drifts almost immediately as external APIs change shape.
|
|
|
|
2. **Cost and latency.** A full eval pass runs the pipeline N times across reviewer topics. Multiplied by every PR (including doc-only PRs), the spend is meaningful and the wall-clock pushes CI from ~30s to many minutes.
|
|
|
|
3. **Non-determinism in the judging path.** The LLM-judged metrics are valuable for review but depend on judge-model behavior on a given day. A flaky eval that fails 1 PR in 20 because the judge re-scored an item differently is a worse CI signal than no eval at all — it teaches contributors to retry rather than read the result.
|
|
|
|
The deterministic overlap metrics are useful regression signals but they are not the same as user-facing correctness. A change that improves overlap can degrade synthesis quality; a change that drops overlap can be a deliberate improvement. So even the deterministic side isn't safe to auto-fail on.
|
|
|
|
## Guidance
|
|
|
|
### 1. Keep search-quality eval available, just not automatic
|
|
|
|
The script stays runnable by maintainers and contributors. The pattern is:
|
|
|
|
```bash
|
|
LAST30DAYS_PYTHON=python3.13 \
|
|
python3 skills/last30days/scripts/evaluate_search_quality.py \
|
|
--baseline main --candidate HEAD
|
|
```
|
|
|
|
Reviewers can request a manual eval run when a PR is in the retrieval/ranking/synthesis path and the risk warrants it. Contributors can run it locally before submitting if they want signal upfront.
|
|
|
|
### 2. Standard PR CI gates remain deterministic and contract-shaped
|
|
|
|
`pytest` (offline-safe), plugin-contract checks, version-consistency contracts, ruff/lint. Anything that returns the same answer twice for the same input. Quality-of-output assessment lives outside that loop.
|
|
|
|
### 3. The middle ground is `workflow_dispatch`, not auto-PR-gating
|
|
|
|
If maintainers want a GitHub-triggered eval that doesn't make every PR pay the live-API cost, the right shape is a manually-dispatched workflow (or a label-triggered one) — not a `pull_request:` workflow that runs unconditionally. That keeps the cost knob in human hands.
|
|
|
|
### 4. Revisit if the eval can ever be made offline-deterministic
|
|
|
|
The blocker is the live-API + non-determinism combination. If a future iteration of the script can compute meaningful Jaccard/retention metrics against static fixtures (no live API calls, no LLM judging), the decision flips and it becomes a candidate for default CI. The decision below tracks that condition; revisit when it's met.
|
|
|
|
## What this means in practice
|
|
|
|
- Don't merge PRs that wire `evaluate_search_quality.py` into the default `validate.yml` workflow.
|
|
- Do merge PRs that add `workflow_dispatch` triggers or label-gated runs.
|
|
- When reviewing a retrieval/ranking change, request a manual eval if the diff suggests it could regress quality — don't expect CI to catch it.
|
|
|
|
## Links
|
|
|
|
- `skills/last30days/scripts/evaluate_search_quality.py` — the evaluator script
|
|
- `docs/search-quality-eval.md` — user-facing usage documentation
|
|
- `.github/workflows/validate.yml` — the default CI workflow (deterministic gates only)
|
|
|
|
---
|
|
|
|
*Adapted from a draft ADR proposed by @hnshah in [#374](https://github.com/mvanhorn/last30days-skill/pull/374), restructured into the `docs/solutions/` convention. The original ADR text correctly identified the constraint; this version adds the "why workflow_dispatch is the middle ground" framing and the revisit-condition.*
|