Skip to content

Add MCP_RECTOR_WARM_SESSION_IGNORE and cover TrackingParser directly - #290

Merged
fdaviddpt merged 3 commits into
mainfrom
fix/200
Oct 3, 2026
Merged

fdaviddpt merged 3 commits into
mainfrom
fix/200

Conversation

@fdaviddpt

Copy link
Copy Markdown
Contributor

Two low-priority follow-ups, both scoped and verified independently.

#200 -- exclude runtime-written directories from the session watch

Adds MCP_RECTOR_WARM_SESSION_IGNORE, the mirror of MCP_RECTOR_WARM_SESSION_WATCH: a PATH-separated list of directories the warm session never walks or lists. Declaring a project's own runtime-written directory (a cache, logs, uploads) there stops it from forcing a session respawn on every call, per the measurement in #200's own issue body.

RectorRunner::sessionWatchPaths() and the new sessionIgnorePaths() now share a splitPathListEnv() helper -- byte-identical parsing behaviour for the pre-existing watch var, confirmed by diffing against the pre-change body. SessionHooks::directorySnapshot() gained a third $ignorePaths parameter, reusing the existing CONTAINS-a-root guard so a misdeclared ignore path can never silently un-watch a real root.

Scope check: traced every dryRun:false write path and every LSP write-shaped dry run -- all routed around the session entirely via forkAndExecute(). directorySnapshot()'s new parameter can only affect WHEN a session is deemed stale, never what any call computes or writes, so this does not need CLAUDE.md's independent warm==cold E2E check; worst case is a stale read-only dry-run diagnostic, which CI already backstops.

Documented in docs/how-it-works.md, including the correctness caveat: never declare a source directory, since its contents feed analysis and a stale listing would serve stale answers with no warning.

#227 -- cover TrackingParser directly; document the rector-warm-lsp coverage gap

No production code changed. A dedicated TrackingParserTest now exercises all three of TrackingParser's methods directly via a hand-built recording fake Parser, asserting both halves the maintainer's own follow-up comment on #227 asked about: the hook fires before the inner parser runs for parseFile() (must-fire), and never fires for parseString() (must-not-fire) -- each test is the other's control.

bin/rector-warm-lsp's 0% coverage (only E2E jobs start it, and they do not set the coverage env vars) is now documented as an accepted, known gap in CONTRIBUTING.md, per the issue's own accept-and-document option; wiring coverage into the E2E jobs is a separate, larger CI change and stays out of scope.

Review

Two reviewers (Explore, oss:auditor) ran against the first two commits. The auditor found one real gap: the new CONTAINS-a-root guard branch for ignore paths had no test exercising the branch where it actually fires. Fixed in the third commit, verified red (guard deliberately broken) then green (restored).

Closes #200, closes #227.

🤖 Generated with Claude Code

Florian DAVID and others added 3 commits October 4, 2026 00:26
…om the session watch (#200)

The warm session's directory-listing watch has no way to exclude a
directory a project writes to at analysis time (a runtime cache under
the analysed roots, not a PHPStan/Rector cache), so it respawns the
session every time that directory's listing changes.

MCP_RECTOR_WARM_SESSION_IGNORE mirrors MCP_RECTOR_WARM_SESSION_WATCH's
format and parsing (RectorRunner::sessionIgnorePaths(), sharing the
PATH_SEPARATOR-splitting helper with sessionWatchPaths()) and threads
the declared paths into SessionHooks::directorySnapshot() as additional
excluded directories, with the same "never exclude a directory that
contains a root" guard the cache dirs already get.

Closes #200.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ge gap (#227)

#227's own follow-up narrowed the remaining work to two items, since the
maintainer's comment already found the session-child-exits-cleanly
hypothesis contradicted by CI (SessionHooks.php 89.4%, TrackingParser.php
50% there, against a 0% that was a local pcov-build artifact):

- TrackingParser.php had no direct unit test at all -- every existing use
  either only built one (SessionHooksTest's installParserHook() tests) or
  stood a fake in for it. parseFile() and parseString() are exactly the
  two methods #227 asks to tell apart: parseFile() reports to the hook
  before delegating, parseString() has no file path to report and never
  calls the hook. The new TrackingParserTest.php covers both, with the
  "must fire" and "must not fire" halves asserted against each other.
- bin/rector-warm-lsp's 0% is accepted (not fixed) per the issue's own
  second option: the E2E/headless jobs that start it never set
  MCP_RECTOR_WARM_COVERAGE_DIR, and wiring that in is a separate, larger
  CI change. CONTRIBUTING.md now documents the gap instead of leaving it
  undocumented.

Closes #227.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
… finding)

The self-review's auditor spawn flagged that directorySnapshot()'s new
ignore-path exclusion carries the same "never exclude a directory that
contains a root" guard the pre-existing cache-dir exclusion has, but
nothing exercised the branch where that guard actually fires. Confirmed
the gap is real by inverting the guard locally and watching the new test
fail, then restoring it and watching the suite go green again.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@fdaviddpt

Copy link
Copy Markdown
Contributor Author

Independent end-to-end check (CLAUDE.md gate)

Ran against head SHA 6ff1c7f8c75c31a3a810ec7d9fda21a0e323898e, in a fresh, independent clone checked out at that commit (separate from the lane's own worktree/tests), with composer install run fresh.

Verdict: PASS.

What was built and run

A new, independent pytest harness (not part of this PR's own test suite) reusing tests/E2E/mcp_harness.py / tests/E2E/test_lsp_initialize.py plumbing to drive the real bin/mcp-rector-warm (MCP) and the LSP binary over real stdio, on fixture projects built fresh for this check:

  1. MCP, session-path exercise of the new var. A plain (non-ignored) write of a .php file under a runtime-cache/ directory inside the project root forces a session respawn (positive control — confirms the harness can see a respawn at all). The identical write, with MCP_RECTOR_WARM_SESSION_IGNORE=runtime-cache set, produces serve only — no respawn. This is the first time the new parameter was actually exercised end-to-end rather than only at the unit level.
  2. MCP, write-path independence. With MCP_RECTOR_WARM_SESSION_IGNORE set AND the session gone stale on an undeclared custom-rule input (config/map.json, same shape as test_warm_session_watch.py), a dryRun:false call via the real MCP tool was applied to the fixture file and compared byte-for-byte against a fresh cold vendor/bin/rector process run on an identically-edited copy. Equal.
  3. MCP, ordinary write sanity. A plain fixture tree, MCP_RECTOR_WARM_SESSION_IGNORE set, dryRun:false tool call applied, full resulting file tree diffed byte-for-byte against a cold rector process run on an identical copy (tree_contents() comparison, not just the one changed file). Equal.
  4. LSP code-action write. Drove the real LSP binary over stdio with MCP_RECTOR_WARM_SESSION_IGNORE=runtime-cache in the server's environment, textDocument/didOpen then textDocument/codeAction, applied the returned WorkspaceEdit to the original file, and compared against a fresh cold vendor/bin/rector process run. Equal (sayHello rename applied identically).

All four cases passed. Also spot-ran ./vendor/bin/phpunit --filter 'SessionHooksTest|RectorRunnerSessionTest' (22 tests, 91 assertions) — green — and the lane's own test_warm_session_watch.py (3 tests) and a baseline scenario (readme-example) — green, confirming the harness and environment are sound independent of this PR's own assertions.

Scope note

This confirms the maintainer's ruling in the PR context was the right call to enforce mechanically: the write paths checked here (#2 -#4) are unaffected by MCP_RECTOR_WARM_SESSION_IGNORE, consistent with sessionCandidate()'s unconditional !$dryRun guard — but that was verified fresh here with an independent harness, not assumed from the scoping argument.

No code was modified or merged; this comment is the only write performed by this check.

@fdaviddpt
fdaviddpt merged commit ce26891 into main Oct 3, 2026
24 checks passed
@fdaviddpt
fdaviddpt deleted the fix/200 branch October 3, 2026 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant