test(session-registry): reader tests set the window pid on every platform (#692) by cdeust · Pull Request #945 · cdeust/Cortex

cdeust · 2026-11-07T11:28:36Z

Summary

Next slice of #692 (class 5, test_session_registry, 5 Windows failures in run 38008915302, job 114084097957):
test_write_and_read_nominal, test_tombstone_reads_as_none, test_ancestor_walk_finds_claude, test_cached_start_signature_hits_after_first_call, test_session_id_change_seen_immediately_despite_cached_signature (assert None == 'stem-abc', assert None == 100, …).

Root cause (test defect, not product): the reader tests chose the window by faking os.getppid. Only the POSIX branch of _window_claude_pid reads it. On Windows the reader takes the ADR-1096 branch, walks the runner's real process table, finds no claude.exe and reads no entry. The same goes for find_claude_ancestor, whose Windows branch ignores the ps probe the test fakes. The reader tests that expect None (corrupted JSON, unknown version, pid mismatch, start-time divergence, dead pid) passed on Windows without reading the file at all.

Change (tests only):

_as_window(monkeypatch, pid) sets sr._window_claude_pid itself, so the registry-file tests exercise the same reader path on every platform.
The four POSIX ancestor-walk tests pin IS_WINDOWS = False, like test_session_registry_posix_root.py and test_posix_ps.py already do. The Windows walk keeps its own tests in test_session_registry_windows.py.
New test_posix_window_is_the_parent_process pins the POSIX mapping that the getppid patches used to cover implicitly.

No production code change.

Refs #692

Type of change

Bug fix (non-breaking change which fixes an issue)

Test plan

pytest tests_py/infrastructure/test_session_registry.py test_session_registry_windows.py test_session_registry_posix_root.py test_posix_ps.py: 55 passed (Linux).
pytest tests_py/infrastructure -k "session_registry or process_ancestry or posix_ps or groomer_identity": 84 passed, 19 skipped.
Windows simulated on Linux (autouse plugin: IS_WINDOWS = True, the ancestor walk finds no claude.exe, as on the CI runner): on main the 5 failing ids of the log fail, and only those; on this branch the same tests plus the new one pass (7 passed).
ruff check / ruff format --check (ruff version from uv.lock) on the touched file: clean. python scripts/check_craftsmanship.py: OK.

Not verified: nothing ran on real Windows. Local env installed from uv.lock without torch/sentence-transformers (download.pytorch.org unreachable here); the touched tests do not import them.

Audit notes

Autopilot routine, no human review yet.
Remaining Windows: full-suite debt of 194 failing tests (measured 2026-10-10), fix order by root cause #692 slices: Windows re-measure, then class 9 one by one.

Coding-standards compliance

§2.2 Layer dependency direction preserved (tests only).
§9 No dead code.

Reviewer checklist

CHANGELOG.md updated under the appropriate section.
No secrets / credentials / PII in the diff.
CI passes on the latest commit (pending).

Generated by Claude Code

…form (#692) The reader tests faked os.getppid, which only the POSIX branch of _window_claude_pid reads. On Windows the reader walked the runner's real process table, found no claude.exe and read no entry: 5 failures in run 38008915302 (job 114084097957), and the tests expecting None passed there without reading the file. They now patch _window_claude_pid; the POSIX ancestor-walk tests pin IS_WINDOWS False; a new test pins the POSIX mapping. Refs #692 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYqm5ErgxixPH4n5uiq

chatgpt-codex-connector · 2026-11-07T14:26:00Z

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-07T14:26:38.839624Z	`0fd1dad`	Draft marked ready
🔒 Security Review	✅ Completed 2026-11-07T14:27:39.980135Z	`0fd1dad`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

claude · 2026-11-07T14:26:26Z

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

PR #945 review (review 1 of 3)

Read the diff and checked it against session_registry.py
Ran five reviews: code quality, test coverage, docs accuracy, security, performance
Checked CI: the Pipeline run on 0fd1dad passed; the re-run is still in progress
Posted the review: test(session-registry): reader tests set the window pid on every platform (#692) #945 (comment)

No Blockers. All five reviews came back clean. The review comment has three notes that need no action before merge.

I could not run pytest or ruff in this job because the commands were not permitted. The result rests on reading the code and on the passing Pipeline run.

Verdict: approve

claude · 2026-11-07T14:28:12Z

Review 1 of 3: PR #945 (commit `0fd1dad`)

I ran five reviews on this PR: code quality, test coverage, docs accuracy, security and performance. None found a Blocker.

I checked the change against mcp_server/infrastructure/session_registry.py:

Reader path: _as_window patches sr._window_claude_pid, and current_window_session looks that name up when it runs. All 13 current_window_session calls in the test file now go through it, so the tests that expect None read the registry file on Windows too.
POSIX pin: session_registry reads host_platform.IS_WINDOWS as an attribute (lines 73, 109, 181), so pinning it in the four ancestor-walk tests and in the new test takes effect.
Windows branch: still covered by test_session_registry_windows.py (the walk, the per-server cache, and the case with no claude.exe ancestor), and cached_window_pid by test_process_ancestry.py.
Cache isolation: this file never reaches process_ancestry._window_pid_cache, and the autouse fixture still clears _start_signature_cache.
Docs: the CHANGELOG entry and the _as_window docstring match the code.
CI: the Pipeline run on 0fd1dad passed. The re-run is still in progress. I could not run pytest in this job because the command was not permitted.

Notes, no action before merge

No test in test_session_registry.py covers _window_claude_pid() returning None on POSIX. The Windows file covers that early return.
The CHANGELOG and _as_window say the Windows reader "walks the real process table". It reads CLAUDE_PID first (ADR-1124). The statement is still true on the runner.
test_ancestor_walk_probe_failure_returns_none tests an empty or malformed probe result, not a failed probe (which raises OSError). This predates the PR.

Verdict: approve

cdeust · 2026-11-07T14:28:26Z

Thanks. On the three notes, no change in this PR:

_window_claude_pid() returning None: the POSIX branch returns os.getppid(), which is never None. The early return only happens on Windows, where test_session_registry_windows.py covers it.
"walks the real process table": the Windows reader tries CLAUDE_PID first (ADR-1124). The runner has no CLAUDE_PID, so it falls through to the walk, and that walk is what the 5 failures measured. I'm leaving the wording, which describes the failing run.
test_ancestor_walk_probe_failure_returns_none: agreed, it tests an empty probe and not an OSError. It predates this PR and is out of its scope.

Generated by Claude Code

cdeust mentioned this pull request Nov 7, 2026

Windows: full-suite debt of 194 failing tests (measured 2026-10-10), fix order by root cause #692

Open

cdeust marked this pull request as ready for review November 7, 2026 14:25

github-actions Bot merged commit 4b1963e into main Nov 7, 2026
32 checks passed

Navigation Menu

Repository navigation

test(session-registry): reader tests set the window pid on every platform (#692) - #945

test(session-registry): reader tests set the window pid on every platform (#692)#945
github-actions[bot] merged 1 commit into
mainfrom
fix/692-session-registry-window-identity

cdeust commented Nov 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 7, 2026

Uh oh!

cdeust commented Nov 7, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cdeust commented Nov 7, 2026

Summary

Type of change

Test plan

Audit notes

Coding-standards compliance

Reviewer checklist

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #945 review (review 1 of 3)

Uh oh!

claude Bot commented Nov 7, 2026

Review 1 of 3: PR #945 (commit 0fd1dad)

Uh oh!

cdeust commented Nov 7, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chatgpt-codex-connector Bot commented Nov 7, 2026 •

edited

Loading

claude Bot commented Nov 7, 2026 •

edited

Loading

Review 1 of 3: PR #945 (commit `0fd1dad`)