diff --git a/docs/research/agent-procedure.md b/docs/research/agent-procedure.md index 7e2b8bafde..0b0c35722b 100644 --- a/docs/research/agent-procedure.md +++ b/docs/research/agent-procedure.md @@ -149,6 +149,11 @@ The helper does not search every code diff or nested source file. Remote paginat reported as such; snapshots are cached outside the checkout for five minutes. Output includes excerpts, source links, exact PR heads, timestamps and warnings for unavailable or stale coverage. No matches does not establish novelty. +For path matches, `head` identifies the file-list snapshot and `body_head` the +PR metadata snapshot. `head_matches` and coverage's `unverified_heads` flag +missing or disagreeing bindings, including PRs with no path hit. Recheck those +PRs before treating the path search as current; offline legacy caches retain +unknown file heads rather than borrowing a head from PR metadata. Canonical positive integers in task overlap terms match PR references, rather than decimal or commit-hash fragments. Explicit positional terms retain literal matching; leading-zero words stay literal. See [reference matching](overlap-task-reference-matching.md) diff --git a/scripts/find_overlap.py b/scripts/find_overlap.py index 6046601240..e8aad6fc21 100644 --- a/scripts/find_overlap.py +++ b/scripts/find_overlap.py @@ -86,7 +86,7 @@ def pull_file_record(pull): connection = page["files"] if connection is None: raise ValueError("PR file continuation unavailable") - return {"number": pull["number"], "paths": sorted(paths), + return {"number": pull["number"], "head": pull["headRefOid"], "paths": sorted(paths), "truncated": connection is None or len(paths) != pull["changedFiles"]} @@ -142,7 +142,8 @@ def path_report(pr_records, file_rows, patterns): pull = pulls.get(row["number"], {}) matches.append({"kind": "pr", "number": row["number"], "title": pull.get("title", ""), "url": pull.get("url") or f"https://github.com/{REPO}/pull/{row['number']}", - "head": pull.get("head"), + "head": row.get("head"), "body_head": pull.get("head"), + "head_matches": bool(row.get("head") and row["head"] == pull.get("head")), "updated_at": pull.get("updated_at"), "draft": pull.get("draft", False), "matched_paths": matched, "changed_paths": len(row["paths"]), "truncated": row.get("truncated", False)}) @@ -258,6 +259,10 @@ def remote_snapshot(cache, offline=False, refresh=False, max_age=300, need_files except (KeyError, ValueError, TypeError): cached = None fresh = cached is not None and age is not None and 0 <= age <= max_age + if name == "pr_files" and cached: + # Old schema-1 caches omitted file heads. Refresh just this source; + # offline/failed refreshes retain the evidence with explicit warnings. + fresh = fresh and all(row.get("head") for row in cached["records"]) if offline: warnings.append(f"{name}: offline; cached data may miss current changes") elif refresh or not fresh: @@ -410,12 +415,21 @@ def main(): if source: coverage["pr_files"]["records"] = len(source["records"]) pulls = snapshot["sources"].get("open_prs", {"records": []})["records"] + heads = {pull["number"]: pull.get("head") for pull in pulls} + file_heads = {row["number"]: row.get("head") for row in source["records"]} + unverified = sorted(number for number in heads.keys() | file_heads.keys() + if not file_heads.get(number) or file_heads[number] != heads.get(number)) + coverage["pr_files"]["unverified_heads"] = unverified + if unverified: + warnings.append(f"pr_files: file-head binding unverified for {len(unverified)} PRs; " + "paths may be stale. Refresh and recheck the heads listed in coverage.") path_matches = path_report(pulls, source["records"], patterns) matches = search(records, terms, args.all_terms, reference_terms) if any(t.strip() for t in terms) else [] if patterns and matches: # With both axes given, a record must match at least one term and one path. - hits = {m["number"]: m["matched_paths"] for m in path_matches} - matches = [dict(m, matched_paths=hits[m.get("number")]) for m in matches + hits = {m["number"]: m for m in path_matches} + matches = [dict(m, **{key: hits[m["number"]][key] + for key in ("matched_paths", "head", "body_head", "head_matches")}) for m in matches if m.get("kind") == "pr" and m.get("number") in hits] if patterns and not any(t.strip() for t in terms): matches = [] @@ -432,7 +446,9 @@ def main(): report["path_patterns"] = patterns report["path_limits"] = ("Path matching covers the changed file paths GitHub reports for open PRs, " "not diff contents; all available file pages are followed. Missing lists or " - "advertised-count shortfalls are flagged truncated.") + "advertised-count shortfalls are flagged truncated. Head identifies the file " + "snapshot; body_head identifies PR metadata. Unverified joins are flagged, " + "including inventories with no matching path.") report["path_matches_total"] = len(path_matches) report["path_matches"] = path_matches[:args.limit] if args.json_output: @@ -455,6 +471,8 @@ def main(): print(f"\n[{match['kind']}] {match['title']}\n{match.get('url', match.get('path'))}") if match.get("head"): print("Head: " + match["head"]) + if match.get("head_matches") is False: + print("File-head binding unverified; body head: " + str(match.get("body_head"))) print(match["excerpt"]) if patterns: print(f"\n{len(path_matches)} open PRs match paths {patterns}; " @@ -464,6 +482,8 @@ def main(): print(f"\n[pr {match['number']}] {match['title']}{flags}\n{match['url']}") if match.get("head"): print("Head: " + match["head"]) + if not match["head_matches"]: + print("File-head binding unverified; body head: " + str(match["body_head"])) print("Matched paths: " + ", ".join(match["matched_paths"])) diff --git a/tests/test_research_onboarding.py b/tests/test_research_onboarding.py index 22346dd554..9c66be065b 100644 --- a/tests/test_research_onboarding.py +++ b/tests/test_research_onboarding.py @@ -116,8 +116,8 @@ def test_local_readme_and_ledger(self): class OverlapPathIndexTests(unittest.TestCase): """Changed-file index for open PRs (discussion 1200 request: search by path).""" - ROWS = [{"number": 7, "paths": ["docs/research/current-status.md", "README.md"], "truncated": False}, - {"number": 8, "paths": ["research/lean-external-review/EXPLAINER.md"], "truncated": True}] + ROWS = [{"number": 7, "head": "a" * 40, "paths": ["docs/research/current-status.md", "README.md"], "truncated": False}, + {"number": 8, "head": "b" * 40, "paths": ["research/lean-external-review/EXPLAINER.md"], "truncated": True}] def test_path_report_exact_glob_substring_and_rejection(self): prs = [{"kind": "pr", "number": 7, "title": "T", "body": "b", "url": "u7", "head": "a" * 40, @@ -153,21 +153,21 @@ def page(rows, more): with patch.object(overlap, "graphql", side_effect=pages) as call: rows = overlap.fetch_pr_files() self.assertEqual(call.call_count, 2) - self.assertEqual(rows[0], {"number": 1, "paths": ["a.txt", "b.txt"], "truncated": False}) - self.assertEqual(rows[1], {"number": 2, "paths": ["c.txt"], "truncated": True}) + self.assertEqual(rows[0], {"number": 1, "head": "abc", "paths": ["a.txt", "b.txt"], "truncated": False}) + self.assertEqual(rows[1], {"number": 2, "head": "abc", "paths": ["c.txt"], "truncated": True}) def test_fetch_pr_files_skips_unavailable_file_lists(self): page = {"repository": {"pullRequests": { "pageInfo": {"hasNextPage": False, "endCursor": None}, - "nodes": [{"number": 376, "files": None}, + "nodes": [{"number": 376, "headRefOid": "abc", "files": None}, {"number": 9, "headRefOid": "abc", "changedFiles": 1, "files": {"pageInfo": {"hasNextPage": False}, "nodes": [{"path": "ok.md"}, None]}}]}}} with patch.object(overlap, "graphql", return_value=page): rows = overlap.fetch_pr_files() - self.assertEqual(rows[0], {"number": 9, "paths": ["ok.md"], "truncated": False}) + self.assertEqual(rows[0], {"number": 9, "head": "abc", "paths": ["ok.md"], "truncated": False}) # An unavailable file list is recorded as empty and flagged, never silently trusted. - self.assertEqual(rows[1], {"number": 376, "paths": [], "truncated": True}) + self.assertEqual(rows[1], {"number": 376, "head": "abc", "paths": [], "truncated": True}) def test_fetch_pr_files_retries_transient_page_failures(self): page = {"repository": {"pullRequests": { @@ -179,7 +179,7 @@ def test_fetch_pr_files_retries_transient_page_failures(self): with patch.object(overlap, "graphql", side_effect=[boom, boom, page]) as call, \ patch.object(overlap.time, "sleep") as slept: rows = overlap.fetch_pr_files() - self.assertEqual(rows, [{"number": 1, "paths": ["a.md"], "truncated": False}]) + self.assertEqual(rows, [{"number": 1, "head": "abc", "paths": ["a.md"], "truncated": False}]) self.assertEqual(call.call_count, 3) self.assertEqual(slept.call_count, 2) with patch.object(overlap, "graphql", side_effect=[boom] * 4), \ @@ -188,7 +188,7 @@ def test_fetch_pr_files_retries_transient_page_failures(self): overlap.fetch_pr_files() def test_pr_files_fetched_only_when_requested_and_cached(self): - rows = [{"number": 3, "paths": ["x.md"], "truncated": False}] + rows = [{"number": 3, "head": "a" * 40, "paths": ["x.md"], "truncated": False}] with tempfile.TemporaryDirectory() as tmp: cache = Path(tmp) / "cache.json" with patch.object(overlap, "gh_json", return_value=[[]]), \ @@ -211,7 +211,7 @@ def test_pr_files_fetched_only_when_requested_and_cached(self): self.assertTrue(any("pr_files" in w for w in warnings)) def test_pr_files_refresh_failure_keeps_old_records(self): - rows = [{"number": 5, "paths": ["keep.md"], "truncated": False}] + rows = [{"number": 5, "head": "a" * 40, "paths": ["keep.md"], "truncated": False}] with tempfile.TemporaryDirectory() as tmp: cache = Path(tmp) / "cache.json" with patch.object(overlap, "gh_json", return_value=[[]]), \ diff --git a/tests/test_research_pr_file_pages.py b/tests/test_research_pr_file_pages.py index 57810ed04e..29df90ae7c 100644 --- a/tests/test_research_pr_file_pages.py +++ b/tests/test_research_pr_file_pages.py @@ -1,5 +1,7 @@ """Changed-path matches beyond the first page and honest incomplete snapshots.""" import copy +import contextlib +import io import json from pathlib import Path import sys @@ -35,7 +37,7 @@ def api(query, variables): return {'repository': {'pullRequest': continuation}} with patch.object(overlap, 'graphql', side_effect=api): row = overlap.pull_file_record(pull) - self.assertEqual(row, {'number': 42, 'paths': paths, 'truncated': False}) + self.assertEqual(row, {'number': 42, 'head': 'a' * 40, 'paths': paths, 'truncated': False}) self.assertEqual([c['cursor'] for c in calls], ['100', '200']) matches = overlap.path_report([], [row], ['research/case/249.py']) self.assertEqual(matches[0]['matched_paths'], [paths[-1]]) @@ -83,6 +85,122 @@ def test_continuation_failure_preserves_previous_complete_cache(self): self.assertEqual(snapshot['sources']['pr_files'], old) self.assertEqual(json.loads(cache.read_text())['sources']['pr_files'], old) + def test_path_heads_are_not_borrowed_from_a_different_body_snapshot(self): + body = {'kind': 'pr', 'number': 42, 'head': 'b' * 40, 'title': 'needle', + 'body': '', 'url': 'https://example.invalid/42'} + files = {'number': 42, 'head': 'a' * 40, 'paths': ['old.py'], 'truncated': False} + match = overlap.path_report([body], [files], ['old.py'])[0] + self.assertEqual(match['head'], 'a' * 40) + self.assertEqual(match['body_head'], 'b' * 40) + self.assertFalse(match['head_matches']) + body['head'] = 'a' * 40 + self.assertTrue(overlap.path_report([body], [files], ['old.py'])[0]['head_matches']) + self.assertEqual(overlap.path_report([], [files], ['old.py'])[0]['head'], 'a' * 40) + + def test_legacy_file_cache_does_not_acquire_a_head_from_body_metadata(self): + body = {'kind': 'pr', 'number': 42, 'head': 'b' * 40, 'title': 'needle'} + files = {'number': 42, 'paths': ['old.py'], 'truncated': False} + match = overlap.path_report([body], [files], ['old.py'])[0] + self.assertIsNone(match['head']) + self.assertEqual(match['body_head'], 'b' * 40) + self.assertFalse(match['head_matches']) + + def test_cli_warns_about_unverified_heads_even_without_a_path_hit(self): + body = {'kind': 'pr', 'number': 42, 'head': 'b' * 40, 'title': 'needle', + 'body': '', 'url': 'https://example.invalid/42'} + for file_head in ('a' * 40, None): + with self.subTest(file_head=file_head), tempfile.TemporaryDirectory() as directory: + cache = Path(directory) / 'cache.json' + files = {'number': 42, 'paths': ['old.py'], 'truncated': False} + if file_head: + files['head'] = file_head + sources = {name: {'fetched_at': overlap.now(), 'complete_pagination': True, + 'records': rows} + for name, rows in [('open_prs', [body]), ('discussions', []), + ('pr_files', [files])]} + raw = json.dumps({'schema_version': 1, 'repository': overlap.REPO, + 'sources': sources}).encode() + cache.write_bytes(raw) + for terms, pattern in [([], 'missing.py'), (['needle'], 'old.py')]: + output = io.StringIO() + argv = ['find_overlap.py', *terms, '--path', pattern, '--offline', + '--json', '--cache', str(cache)] + with (patch.object(overlap, 'ROOT', Path(directory) / 'repo'), + patch.object(sys, 'argv', argv), contextlib.redirect_stdout(output), + patch.object(overlap, 'gh_json', side_effect=AssertionError('network'))): + overlap.main() + report = json.loads(output.getvalue()) + self.assertEqual(report['coverage']['pr_files']['unverified_heads'], [42]) + self.assertTrue(any('file-head binding unverified' in w for w in report['warnings'])) + if terms: + match = report['matches'][0] + self.assertEqual(match['head'], file_head) + self.assertEqual(match['body_head'], body['head']) + self.assertFalse(match['head_matches']) + else: + self.assertEqual(report['path_matches_total'], 0) + self.assertEqual(cache.read_bytes(), raw) + + def test_cli_covers_missing_inventory_and_matching_head_control(self): + body = {'kind': 'pr', 'number': 42, 'head': 'a' * 40, 'title': 'needle', 'body': ''} + bound = {'number': 42, 'head': 'a' * 40, 'paths': ['new.py'], 'truncated': False} + for files, expected in [([], [42]), ([bound], [])]: + with self.subTest(files=files), tempfile.TemporaryDirectory() as directory: + sources = {name: {'fetched_at': overlap.now(), 'complete_pagination': True, + 'records': rows} + for name, rows in [('open_prs', [body]), ('discussions', []), + ('pr_files', files)]} + cache = Path(directory) / 'cache.json' + cache.write_text(json.dumps({'schema_version': 1, 'repository': overlap.REPO, + 'sources': sources}), encoding='utf-8') + output = io.StringIO() + with (patch.object(overlap, 'ROOT', Path(directory) / 'repo'), + patch.object(sys, 'argv', ['find_overlap.py', '--path', 'new.py', + '--json', '--cache', str(cache)]), + contextlib.redirect_stdout(output), + patch.object(overlap, 'gh_json', side_effect=AssertionError('network'))): + overlap.main() + report = json.loads(output.getvalue()) + self.assertEqual(report['coverage']['pr_files']['unverified_heads'], expected) + if expected: + self.assertTrue(any('file-head binding unverified' in w for w in report['warnings'])) + else: + self.assertEqual(report['warnings'], []) + self.assertTrue(report['path_matches'][0]['head_matches']) + + def test_failed_legacy_refresh_preserves_unknown_head_instead_of_relabelling(self): + with tempfile.TemporaryDirectory() as directory: + cache = Path(directory) / 'cache.json' + old = {'fetched_at': overlap.now(), 'complete_pagination': True, + 'records': [{'number': 42, 'paths': ['old.py'], 'truncated': False}]} + sources = {name: {'fetched_at': overlap.now(), 'complete_pagination': True, 'records': []} + for name in ('open_prs', 'discussions')} + sources['pr_files'] = old + raw = json.dumps({'schema_version': 1, 'repository': overlap.REPO, 'sources': sources}) + cache.write_text(raw, encoding='utf-8') + with patch.object(overlap, 'fetch_pr_files', side_effect=ValueError('later page failed')): + snapshot, warnings = overlap.remote_snapshot(cache, need_files=True) + self.assertTrue(any('pr_files: refresh failed' in w for w in warnings)) + self.assertEqual(snapshot['sources']['pr_files'], old) + self.assertEqual(cache.read_text(encoding='utf-8'), raw) + + def test_fresh_legacy_cache_is_refetched_without_forcing_a_body_refresh(self): + with tempfile.TemporaryDirectory() as directory: + cache = Path(directory) / 'cache.json' + sources = {name: {'fetched_at': overlap.now(), 'complete_pagination': True, + 'records': rows} + for name, rows in [('open_prs', []), ('discussions', []), + ('pr_files', [{'number': 42, 'paths': ['old.py']}])]} + cache.write_text(json.dumps({'schema_version': 1, 'repository': overlap.REPO, + 'sources': sources}), encoding='utf-8') + replacement = [{'number': 42, 'head': 'c' * 40, 'paths': ['new.py'], 'truncated': False}] + with (patch.object(overlap, 'gh_json', side_effect=AssertionError('body refresh')), + patch.object(overlap, 'fetch_pr_files', return_value=replacement) as fetch): + snapshot, warnings = overlap.remote_snapshot(cache, need_files=True) + fetch.assert_called_once_with() + self.assertEqual(warnings, []) + self.assertEqual(snapshot['sources']['pr_files']['records'], replacement) + if __name__ == '__main__': unittest.main()