From 3a3f46acd234583daa1bf078505164aa32be59c1 Mon Sep 17 00:00:00 2001 From: MSCodeBase Agent Date: Tue, 10 Nov 2026 22:39:30 +0300 Subject: [PATCH] fix(search): hashes and commit trailers are not identifiers Found by the external benchmark on sqlalchemy: the tool scored 0/40 at hit@5 while the same tool with neighbours shown scored 18. Not a ranking problem - `_identifiers` took the Gerrit `Change-Id` (a 40-char hash) as the first identifier, the gate counted `Iaf8be... = 0` occurrences in the source, and every commit carrying one abstained. Before: identifiers: Iaf8be4a9..., _ConnectionRecord.__close, run_scenario, Fixed verdict: abstain (0.00) After: identifiers: _ConnectionRecord.__close, run_scenario, AnyIO, ... verdict: answer (0.75), gold at row 2 The fix drops hashes (32+ hex, with or without the Gerrit `I` prefix) and commit trailers (fixed, fixes, issue, issues, closes, resolves, changelog) from `_identifiers`. `close` is deliberately kept: it is a real method name. Measured: sqlalchemy, 40 commits hit@5 before -> after tool 0 -> 11 neighbours 18 -> 19 search 18 -> 18 grep 20 -> 20 B1 dev (71 solvable) 19 / 38 / 22 / 2 -> identical The tool is still below grep on that external repo after the fix, so the small/medium-repo conclusion stands; what changed is that the abstain is no longer systematic. --- AGENT_DIARY.md | 3 ++ EXPERIMENTS_LOG.md | 24 +++++++++ experiments/bench_2026_11/REPORT.md | 52 ++++++++++++++++++- .../results_external_sqlalchemy.json | 24 +++++++++ src/mcp/tools/investigate_tool.py | 10 ++++ tests/test_identifiers.py | 46 ++++++++++++++++ 6 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 experiments/bench_2026_11/results_external_sqlalchemy.json create mode 100644 tests/test_identifiers.py diff --git a/AGENT_DIARY.md b/AGENT_DIARY.md index f727f299..2bcddfc8 100644 --- a/AGENT_DIARY.md +++ b/AGENT_DIARY.md @@ -57,6 +57,9 @@ Пункты «RU→EN словарь» и «concept→symbol словарь» сняты: их заменяет контракт. - **Урок:** не учить сервер языкам, которые знает хост; отдавать `hints` и честно просить их, когда в вопросе нет code-имени. +- **E71 (Change-Id):** хеш из Gerrit занимал первый слот идентификаторов, гейт считал + `0` вхождений и отказывал на каждом коммите sqlalchemy (0/40). Фильтр хешей и + трейлеров поднял tool до 11/40, B1 dev не изменился (19/38/22/2). - **E70 (внешний репо):** на `psf/requests` grep берёт 19/25 hit@5, search 15, neighbours 13, наш tool 10. На маленьком корпусе оркестрация проигрывает обычному поиску по тексту; «мы лучше grep» верно только для символьного поиска на нашей панели. diff --git a/EXPERIMENTS_LOG.md b/EXPERIMENTS_LOG.md index e56295d6..dbd33cc8 100644 --- a/EXPERIMENTS_LOG.md +++ b/EXPERIMENTS_LOG.md @@ -1,4 +1,28 @@ # EXPERIMENTS_LOG.md — Audit Verification (2026-07-22) +## [2026-11-10] — E71: a Gerrit `Change-Id` made the gate abstain on every commit + +**Found by** the sqlalchemy run of B2: tool 0/40 at hit@5 while neighbours got +18. Not a ranking result - `_identifiers` took the 40-char `Change-Id` hash as +the first identifier, the gate counted `0` occurrences, and the answer was +`abstain` on every commit that carried one. + +**Fix:** hashes (32+ hex, with or without the Gerrit `I` prefix) and commit +trailers (`fixed`, `fixes`, `issue`, `closes`, `resolves`, `changelog`) are no +longer identifiers; `close` stays, it is a real method name. + +``` +sqlalchemy, 40 commits hit@5 before -> after +tool 0 -> 11 +neighbours 18 -> 19 +search 18 -> 18 +grep 20 -> 20 +B1 dev (71) 19/38/22/2 -> identical +``` + +After the fix the tool is still below grep on this external repo (11 vs 20), so +the small/medium-repo conclusion stands; what changed is that the abstain is no +longer systematic. + ## [2026-11-10] — E70: external repo (psf/requests) — plain grep wins **Why:** B1 still lives in our own repository. Here the repo, history, messages diff --git a/experiments/bench_2026_11/REPORT.md b/experiments/bench_2026_11/REPORT.md index 761a621a..36b97a55 100644 --- a/experiments/bench_2026_11/REPORT.md +++ b/experiments/bench_2026_11/REPORT.md @@ -141,8 +141,40 @@ visible in the log before the crash; no code, arm or metric changed after that (the arms were frozen in Amendments 1-3 before the sealed run), so this is disclosed rather than "clean". -## B2 — external repository (`psf/requests`) +## B2 — external repository: `sqlalchemy` (the finding that mattered) + +679 `.py` files, corpus at `8b62343` (2026-09-24), 34 984 chunks, one index +build (53 min). 40 commits. + +``` + hit@1 hit@5 hit@1 hit@5 + before E71 after E71 +tool 0 0 8 11 +neighbours 13 18 11 19 +search 12 18 12 18 +grep 7 20 7 20 +``` + +**The zero was a bug, not a small-repo effect.** `_identifiers` took the Gerrit +`Change-Id` (a 40-char hash) as the first identifier, the gate counted +`Iaf8be… = 0` occurrences, and every such commit abstained: + +``` +before: identifiers: Iaf8be4a9…, _ConnectionRecord.__close, run_scenario, Fixed + verdict: abstain (0.00) +after: identifiers: _ConnectionRecord.__close, run_scenario, AnyIO, sqlalchemy.testing.cancellation + verdict: answer (0.75), gold at row 2 +``` + +E71 drops hashes (32+ hex, with or without the Gerrit `I` prefix) and commit +trailers (`fixed`, `fixes`, `issue`, `closes`, …) from `_identifiers`; `close` +stays, because it is a real method name. B1 dev is **identical** after the fix +(19 / 38 / 22 / 2), so the calibrated corpus did not move. +After the fix the tool is still below grep here (11 vs 20) — the small-repo +conclusion from `requests` survives — but the catastrophic abstain is gone. + +## B2 — external repository (`psf/requests`) Nothing in this run is ours: the repository, its history, its commit messages and the gold all come from other people. Same rule as B1; corpus at `e331a288` (2026-03-24), 64 files / 786 chunks, one index build (16 s). @@ -184,6 +216,24 @@ split was re-run on the merged build: **identical** (`tool` 19, `neighbours` 38, `search` 22, `grep` 2; paired +16). The fix does not touch this benchmark, which is what a classification change should look like. +## Reproduce + +``` +# B1 corpus (this repo at the frozen base) +git worktree add D:\Project\bench-B 7c2c7a3c6417539bc08f807d6790fe6e722d3a84 +python scripts/reindex_text_only.py --project D:\Project\bench-B +python experiments/bench_2026_11/commit_tasks.py +python experiments/bench_2026_11/run.py --split dev +python experiments/bench_2026_11/run.py --split sealed --sealed # once per release + +# B2 corpus (external) +git clone https://github.com/psf/requests D:\Project\bench-ext\requests +git -C D:\Project\bench-ext\requests worktree add D:\Project\bench-ext\req-B e331a288 +python scripts/reindex_text_only.py --project D:\Project\bench-ext\req-B +python experiments/bench_2026_11/external.py --repo D:\Project\bench-ext\requests \ + --corpus D:\Project\bench-ext\req-B --base e331a288 --limit 40 +``` + ## Threats - The corpus is one snapshot at `B`. A file renamed after `B` makes its task diff --git a/experiments/bench_2026_11/results_external_sqlalchemy.json b/experiments/bench_2026_11/results_external_sqlalchemy.json new file mode 100644 index 00000000..86ab85f3 --- /dev/null +++ b/experiments/bench_2026_11/results_external_sqlalchemy.json @@ -0,0 +1,24 @@ +{ + "repo": "sqlalchemy", + "base": "8b62343934387efcc68f02032fa6cc07ac65ba26", + "tasks": 40, + "table": { + "tool": { + "top1": 8, + "top5": 11 + }, + "neighbours": { + "top1": 11, + "top5": 19 + }, + "search": { + "top1": 12, + "top5": 18 + }, + "grep": { + "top1": 7, + "top5": 20 + } + }, + "invalid": 0 +} \ No newline at end of file diff --git a/src/mcp/tools/investigate_tool.py b/src/mcp/tools/investigate_tool.py index 5a508fdd..fa0ab79b 100644 --- a/src/mcp/tools/investigate_tool.py +++ b/src/mcp/tools/investigate_tool.py @@ -51,6 +51,10 @@ _MAX_IDS = 4 _CHANNEL_TIMEOUT = 12.0 _GREP_TIMEOUT = 15.0 +# Hashes are not identifiers: a Gerrit `Change-Id` (40 hex chars) or a commit +# sha can never occur in the source, and counting them made the gate abstain on +# every sqlalchemy commit (E71, external benchmark: 0/40 at hit@5). +_HEX_TOKEN_RE = re.compile(r"^I?[0-9a-fA-F]{32,}$") _STOPWORDS = { "the", "and", "for", "how", "why", "what", "does", "used", "use", "where", "when", "which", "with", "from", "into", "that", "this", "files", "file", @@ -58,6 +62,10 @@ "who", "whom", "whose", "defined", "definition", "define", "defines", "implemented", "implement", "implements", "located", "sits", "lives", "raises", "raise", "returns", "return", "callers", "caller", "usages", + # Commit trailers (Gerrit/GitHub) are not code names; counting them made the + # gate abstain on prose (E71). `close` is left out on purpose: it is a real + # method name. + "fixed", "fixes", "issue", "issues", "resolves", "closes", "changelog", } @@ -90,6 +98,8 @@ def _identifiers(query: str) -> List[str]: token = token.strip(".") if not token or token.lower() in _STOPWORDS: continue + if _HEX_TOKEN_RE.match(token): + continue if token not in found: found.append(token) diff --git a/tests/test_identifiers.py b/tests/test_identifiers.py new file mode 100644 index 00000000..f5f347e4 --- /dev/null +++ b/tests/test_identifiers.py @@ -0,0 +1,46 @@ +"""Identifiers extracted from a question must not be hashes or commit trailers. + +E71, discovered by the external benchmark: a Gerrit `Change-Id` took the first +slot in `_identifiers`, the gate counted it as "0 occurrences", and every +sqlalchemy commit abstained (0/40 at hit@5). +""" + +from __future__ import annotations + +from src.mcp.tools.investigate_tool import _identifiers + +GERRIT_MESSAGE = ( + "discard pooled connection when its close is interrupted\n\n" + "Fixed issue where a task was cancelled while closing.\n\n" + "Fixes: #13673\n" + "Change-Id: Iaf8be4a9f4cc0f4435ae97ef8d961ce89375ffcc\n" +) + + +def test_gerrit_change_id_is_not_an_identifier(): + ids = _identifiers(GERRIT_MESSAGE) + + assert not any(len(i) >= 32 for i in ids), ids + assert "Iaf8be4a9f4cc0f4435ae97ef8d961ce89375ffcc" not in ids + + +def test_commit_trailers_are_not_identifiers(): + ids = [i.lower() for i in _identifiers(GERRIT_MESSAGE)] + + for word in ("fixed", "fixes", "issue", "issues", "closes", "resolves"): + assert word not in ids, ids + + +def test_a_real_method_name_still_survives(): + ids = _identifiers("where is _ConnectionRecord.__close defined") + + assert ids == ["_ConnectionRecord.__close"] + + +def test_close_is_still_an_identifier(): + # Left out of the stopword list on purpose: `close` is a real method name. + assert "close" in _identifiers("where is close defined") + + +def test_a_plain_question_still_yields_its_identifier(): + assert _identifiers("where is get_progress_file defined") == ["get_progress_file"]