fix(tests): send platform-absolute event paths in the preemptive-context tests (#692, class 5) - #920

Claude finished @cdeust's task in 1m 42s —— View job

Review of PR #920 (review 1 of 3)

• Read the diff and the hook it tests (mcp_server/hooks/preemptive_context.py)
• Ran the five reviewers (code quality, performance, test coverage, docs accuracy, security)
• Checked their findings myself
• Posted the review: fix(tests): send platform-absolute event paths in the preemptive-context tests (#692, class 5) #920 (comment)

No Blockers. On Windows, C:\a.py passes is_absolute(), and .resolve() returns the same string, so assert_called_once_with(A_PY, None) still matches. project is None in these tests, so the seeded _cooldown_key(A_PY, None) is the key the hook actually looks up. The "/a.py" strings that are left are only used as plain cooldown keys or passed to a mock, so they don't fail on Windows.

I posted two Notes, neither needed before merge:

• A sibling test seeding a fresh cooldown would catch the case where the seed is silently ignored again.
• The fix hasn't been run on Windows yet.

Verdict: approve

Review 1/3: PR #920

Five reviewers ran (code quality, performance, test coverage, docs accuracy, security). I traced their claims against mcp_server/hooks/preemptive_context.py and mcp_server/shared/project_scope.py. None of them found a Blocker.

What I checked:

• The assertion holds on Windows. A_PY = str(Path("/").resolve() / "a.py") gives something like C:\a.py. That passes is_absolute() at preemptive_context.py:134. Line 136 computes Path("/") / "C:\a.py"; the joined path has a drive, so it replaces the left side and .resolve() returns the same string. So assert_called_once_with(A_PY, None) still matches.
• project is really None in these tests. resolve_project_root only reads CLAUDE_PROJECT_ROOT, which the autouse fixture removes, and event["cwd"], which no test event sets.
• The expired-cooldown test now tests expiry. The seeded key now equals json.dumps([None, A_PY]), the key _check_cooldown actually looks up. The old "/a.py" seed was never read.
• The remaining "/a.py" strings are safe. The ones in the _check_cooldown, _update_cooldown and store tests are opaque keys or are passed to a mock. None of them goes through _event_paths, so they do not fail on Windows.
• The CHANGELOG entry and the new comment are accurate. No security or performance concerns: the only file the tests write goes to tmp_path.

Notes, no action before merge

• test_expired_cooldown_primes_again would still pass if the seed were silently ignored again, for example after another key-format change. A sibling test that seeds a fresh _cooldown_key(A_PY, None) and asserts prime.assert_not_called() would pin that down.
• Nothing has run on Windows yet; the PR body says so. The next Windows full-suite run will confirm the fix.

Verdict: approve

Sur la note non bloquante de la revue 1/3 (test frère avec une clé fraîche) : je ne la pousse pas dans cette PR. La graine est maintenant construite par hook._cooldown_key, la fonction même que process_event utilise pour la lecture : un changement de format de clé change les deux côtés à la fois et ne peut plus rendre la graine silencieusement ignorée. Le seul écart restant serait que process_event cesse d'appeler _cooldown_key, et test_second_event_within_the_window_is_skipped couvre déjà le comportement « cooldown frais, pas de second priming ». Le test proposé reste une amélioration possible, pas un défaut de cette PR.

Generated by Claude Code