fix(network-intercept): evict session cache on HTTP delete

[shaun0927]

Summary

• Exposes an optional MCPTransport.onSessionDelete lifecycle hook through the transport abstraction.
• Wires HTTP session deletion to removeNetworkInterceptorForSession(sessionId) so network_intercept per-session rule state is evicted when streamable HTTP sessions are deleted.
• Adds unit coverage for direct cache eviction and MCPServer lifecycle wiring.

Issue analysis / scope

Open issue #763 is still valid: HTTP transport already has a session delete callback, but MCPServer did not register cleanup, so the module-level network interceptor map retained per-session NetworkInterceptor instances after DELETE /mcp.

This PR intentionally keeps the scope narrow:

• It removes stale in-memory interceptor state for the deleted session.
• It preserves stdio behavior by making the transport hook optional.
• It avoids a mcp-server -> network-intercept import cycle by moving the cache into a side-effect-light module that both layers can share synchronously.

Validation

• npx jest --runTestsByPath tests/unit/network-intercept-tool.test.ts tests/unit/mcp-server-session-cleanup.test.ts --runInBand
• npx eslint --max-warnings=0 src/tools/network-intercept.ts src/tools/network-intercept-cache.ts src/transports/index.ts src/mcp-server.ts tests/unit/network-intercept-tool.test.ts tests/unit/mcp-server-session-cleanup.test.ts
• npx tsc --noEmit --pretty false

Closes #763

[shaun0927]

PR Review Summary

Verdict

Approve

Scope Reviewed

• PR intent: close Evict NetworkInterceptor map entries on MCP session deletion (HTTP transport) #763 by evicting per-session network interceptor cache entries when HTTP MCP sessions are deleted.
• Main changed areas: transport lifecycle interface, MCPServer session-delete wiring, extracted network-intercept cache module, targeted unit tests.
• Tests reviewed: direct cache isolation/eviction tests and MCPServer registration test.
• Checks considered: local jest/eslint/tsc passed; GitHub dependency-audit/lint/test passed, build pending at review time.

Blocking Issues

None.

Warnings

None.

Mutation-Test Thinking

• Likely mutants that should be killed: deleting the wrong session id, ignoring empty ids, failing to register onSessionDelete, replacing synchronous removal with no-op.
• Mutants current tests may not catch: future composite-key device scopes beyond the current session|... prefix convention if the key format changes.
• Additional tests recommended: none required for this PR scope.

Complexity / CRAP-style Risk

• High-risk functions/modules: low; cleanup is isolated in network-intercept-cache.ts.
• Complexity increase: minimal optional transport hook plus one cache helper.
• Test coverage concern: none for stated scope.
• Refactoring recommendation: none.

Test Quality Assessment (6/7)

• Strong tests: prove per-session eviction preserves other session rules and MCPServer wires the transport delete hook.
• Weak tests: no live HTTP DELETE smoke, but local unit coverage is appropriate for a narrow lifecycle wiring change.
• Missing edge cases: no blocking gap.
• Mocking concerns: MCPServer transport mock is narrow and directly exercises registration behavior.

Security / Operational Risk

None. The change reduces long-lived state retention; no auth or external input surface is widened.

Looks Good

• Synchronous cache cleanup avoids DELETE/reuse races.
• Optional transport hook keeps stdio behavior unchanged.
• Cache extraction removes the import-cycle risk cleanly.

Final Recommendation

Approve. Merge once the pending GitHub build check completes successfully.

[shaun0927]

Ready to merge — approve summary

All required CI checks are green on the latest commit 6a3bf234 (lint, test, dependency-audit, build — all SUCCESS). No P0/P1 findings from any bot (no Codex review comments on this PR after the inherited findings were addressed). Local verification on the same SHA: npx tsc --noEmit --pretty false exits 0, npx eslint --max-warnings=0 on the six touched files exits 0, and npx jest --runTestsByPath tests/unit/network-intercept-tool.test.ts tests/unit/mcp-server-session-cleanup.test.ts --runInBand reports Tests: 6 passed, 6 total. This summary records why the PR is in a mergeable state.

What this PR is

The follow-up to #763 that was split out during the v0.6.1 release (PR #762, Codex P2 #3) because the cleanest fix needed new MCP-server-side session-lifecycle wiring. Issue #763 stayed open with full repro and a proposed fix outline; this PR implements that outline:

1. Introduces an optional onSessionDelete(handler) hook on the MCPTransport interface (src/transports/index.ts:28-34).
2. MCPServer.start() calls this.transport.onSessionDelete?.(sessionId => removeNetworkInterceptorForSession(sessionId)) (src/mcp-server.ts:200-202).
3. Extracts the existing per-session interceptor cache from src/tools/network-intercept.ts into a new src/tools/network-intercept-cache.ts module so that both mcp-server.ts and network-intercept.ts can call into it without re-introducing a mcp-server → network-intercept → mcp-server import cycle.

The HTTP transport already exposed an onSessionDelete lifecycle callback (src/transports/http.ts:65-67); it just had no caller. After this PR it does, and the per-session NetworkInterceptor instance is evicted synchronously the moment DELETE /mcp lands.

Scope / direction review

Axis	Verdict
Aligned with project direction	✅ Closes the only deferred Codex finding from #762's release; matches the existing per-session scoping pattern from develop's #753 series.
Single concern	✅ One concern: evict per-session interceptor state on HTTP session deletion. Six files, +125/−21 net, no drive-by refactors beyond the cycle-breaking module extraction the new MCP-server caller forces.
Over-engineering risk	✅ Considered and rejected: (a) plumbing a generic "session-lifecycle event bus" (not needed for a single subscriber — the optional hook is the simplest stable surface), (b) eviction inside HTTPTransport itself (would couple transport code to a tool's cache — wrong layer), (c) deferring eviction to next access via TTL (loses the determinism we want and makes test assertions hard).
Public API surface change	✅ Additive only: MCPTransport.onSessionDelete? is optional, so stdio and any other stateless transport keep working unchanged; the new removeNetworkInterceptorForSession(sessionId) helper and the re-extracted getNetworkInterceptorForSession / networkInterceptor / resetNetworkInterceptorsForTest symbols are still re-exported from src/tools/network-intercept.ts, so existing imports continue to resolve.

Why merge is OK

• Cycle-safe extraction. network-intercept-cache.ts has zero dependencies on mcp-server.ts (it only imports NetworkInterceptor), so mcp-server.ts → network-intercept-cache.ts is a clean acyclic edge. network-intercept.ts re-exports the same symbols so call sites are unchanged.
• Synchronous eviction matches the lifecycle. removeNetworkInterceptorForSession is plain Map.delete work; there's no async hop between "DELETE /mcp received" and "cache entry gone", which closes the DELETE/reuse race that motivated Evict NetworkInterceptor map entries on MCP session deletion (HTTP transport) #763.
• Composite-key safe. The cache key format is <sessionId> or <sessionId>|<deviceId> (the latter introduced in chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762 / fabe0ab1). Eviction strips both key === sessionId and key.startsWith(\${sessionId}|`)`, so the device-scoped extension stays compatible.
• STDIO behaviour preserved. onSessionDelete is optional?: on the MCPTransport interface and the call site uses this.transport.onSessionDelete?.(...). The stdio transport doesn't define it, so its behaviour is byte-identical to before.
• Defense-in-depth, not just memory hygiene. Beyond the GC win, this prevents stale InterceptRules from a previous session from being served on a freshly minted same-id reuse (an HTTP-transport corner case that would otherwise depend on identity reuse semantics).

Verification on 6a3bf234

$ npx tsc --noEmit --pretty false           # exit 0
$ npx eslint --max-warnings=0 \
    src/tools/network-intercept.ts \
    src/tools/network-intercept-cache.ts \
    src/transports/index.ts src/mcp-server.ts \
    tests/unit/network-intercept-tool.test.ts \
    tests/unit/mcp-server-session-cleanup.test.ts   # exit 0
$ npx jest --runTestsByPath \
    tests/unit/network-intercept-tool.test.ts \
    tests/unit/mcp-server-session-cleanup.test.ts --runInBand
Tests:       6 passed, 6 total

GitHub CI on the same commit: lint ✅ · test ✅ · dependency-audit ✅ · build ✅.

Test coverage added

Test	What it locks
evicts only the selected MCP session cache on session deletion	removeNetworkInterceptorForSession('session-a') returns 1, drops the session-a interceptor (subsequent getNetworkInterceptorForSession('session-a') returns a new instance with no rules), and leaves session-b rules intact. Kills the obvious "delete wrong session" / "no-op delete" mutants.
reports no eviction for unknown or empty session ids	Returns 0 for 'missing' and '', doesn't touch existing entries. Kills the "delete everything on empty string" mutant.
registers network interceptor eviction for transport session deletion (MCPServer)	Confirms MCPServer.start({ transport: 'http' }) wires onSessionDelete(handler) and that calling handler('session-a') triggers removeNetworkInterceptorForSession('session-a'). Kills "forgot to register" and "called with wrong sessionId" mutants.

Out-of-scope follow-ups (not blocking this PR)

1. Live HTTP DELETE smoke test. Documented gap; unit coverage on the wiring point is appropriate for a narrow lifecycle change.
2. Other per-session caches (if any are added later). They can register their own subscribers via the same optional hook.

Approving on the strength of green CI, zero bot findings, the scope-discipline above, and the wiring + cache-isolation tests that lock both the registration point and the eviction semantics.

[shaun0927]

PR #764: fix(network-intercept): evict session cache on HTTP delete

/pr-review-os skill pass — 6-axis (WebKit / iOS / Simulator / MCP / Architecture / Reliability) sweep on the latest commit 6a3bf234.

P0 — Blockers (must fix before merge)

None.

P1 — Must Fix (should fix in this PR)

None.

P2 — Improve (can be follow-up)

None at the >= 60/100 confidence threshold.

Per-axis findings

Axis	Finding
WebKit Protocol	Not applicable — this PR is MCP-server-side session-cache plumbing. No WebKit Remote Debugging Protocol calls are added or modified. The NetworkInterceptor class itself is untouched; only the per-session caching layer in front of it is reorganised.
iOS Safari Compatibility	Not applicable — no DOM, no JS-injection payload changes. The interceptor's injected fetch / XMLHttpRequest override script in src/network-interceptor.ts is byte-identical post-PR.
Simulator	Not applicable — no simctl calls added. Device scoping uses the existing device_id parameter pattern.
MCP Protocol	✅ src/transports/index.ts adds onSessionDelete?(handler) as an optional member of the MCPTransport interface, so stdio (and any future stateless transport) keeps working without a method-not-found at call time — the call site in mcp-server.ts:200 uses this.transport.onSessionDelete?.(...). src/transports/http.ts:65-67 already exposed the matching method; lines 501-502 fire it on DELETE /mcp. No console.log calls in any src/ file touched by this PR (the only console.error use is in http.ts:38, which is pre-existing and correct for stderr logging). tools/list response shape is unchanged (the network_intercept tool's inputSchema is untouched).
Architecture	✅ No puppeteer-core / CDP / Chrome imports. The new src/tools/network-intercept-cache.ts module has a single non-type import (NetworkInterceptor); mcp-server.ts → network-intercept-cache.ts is a clean acyclic edge (the new cache module never imports mcp-server.ts, sidestepping the mcp-server → network-intercept → mcp-server cycle that would have happened if cache code stayed in network-intercept.ts). network-intercept.ts re-exports the moved symbols so all existing import sites continue to resolve. Types are explicit (number return on removeNetworkInterceptorForSession, Map<string, NetworkInterceptor> cache); no any.
Reliability	✅ removeNetworkInterceptorForSession is synchronous Map.delete work — no async hop between "DELETE /mcp received" and "cache entry gone", which closes the DELETE/reuse race that motivated #763. Composite-key safe: strips both key === sessionId and key.startsWith(\${sessionId}

Wiring proof

• HTTP DELETE path: src/transports/http.ts:212 (case 'DELETE') → src/transports/http.ts:501-502 (this.sessionDeleteHandler(sessionId)).
• Registration: src/mcp-server.ts:200-202 (this.transport.onSessionDelete?.((sessionId) => removeNetworkInterceptorForSession(sessionId))).
• Cache eviction: src/tools/network-intercept-cache.ts:20-31.

End-to-end: a DELETE /mcp with the deleted session's id reaches removeNetworkInterceptorForSession(sessionId) synchronously, dropping <sessionId> and every <sessionId>|<deviceId> entry from interceptorsBySession. Stdio sessions skip the optional hook entirely.

Test coverage delta (+3 unit tests, +1 wiring test)

Test	Mutant killed
evicts only the selected MCP session cache on session deletion	(a) returning the wrong removed-count, (b) deleting the wrong session, (c) deleting both sessions on a single call, (d) returning the cached interceptor instead of a freshly constructed one for a re-acquired sessionId after delete.
reports no eviction for unknown or empty session ids	(a) deleting everything when sessionId is the empty string, (b) returning a non-zero count for unknown sessions.
registers network interceptor eviction for transport session deletion (MCPServer)	(a) forgetting to register onSessionDelete, (b) registering it with a closure that drops the sessionId, (c) calling removeNetworkInterceptorForSession() with a hard-coded constant rather than the handler argument.
(Existing) clear/disable restores and clears only the selected session	Anchors the per-session scoping the new eviction extends.

Verification

$ npx tsc --noEmit --pretty false           # exit 0
$ npx eslint --max-warnings=0 \
    src/tools/network-intercept.ts \
    src/tools/network-intercept-cache.ts \
    src/transports/index.ts src/mcp-server.ts \
    tests/unit/network-intercept-tool.test.ts \
    tests/unit/mcp-server-session-cleanup.test.ts   # exit 0
$ npx jest --runTestsByPath \
    tests/unit/network-intercept-tool.test.ts \
    tests/unit/mcp-server-session-cleanup.test.ts --runInBand
Test Suites: 2 passed, 2 total
Tests:       6 passed, 6 total

GitHub CI on 6a3bf234: lint ✅ · test ✅ · dependency-audit ✅ · build ✅.

Summary

Priority	Count
P0	0
P1	0
P2	0

Verdict

APPROVE.

Rationale, in order of weight:

1. Zero blocking findings on any of the six review axes; the diff is 6 files, +125/−21 net, with no drive-by changes beyond the cycle-breaking module extraction that the new MCP-server caller forces.
2. Closes the only deferred Codex finding from the v0.6.1 release (chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762 Codex P2 Epic 0B: OpenChrome Shared Module Extraction #3), exactly along the fix outline that Evict NetworkInterceptor map entries on MCP session deletion (HTTP transport) #763 captured. The wiring lands at the layer the issue body proposed: an optional transport hook, registered by MCPServer.start(), draining into a stateless cache helper.
3. No layering violation introduced. network-intercept-cache.ts is a leaf module (one import: NetworkInterceptor); mcp-server.ts → network-intercept-cache.ts is the new edge — clean and acyclic. The transport interface stays transport-agnostic (optional hook, stdio unaffected).
4. Synchronous, deterministic eviction. No timers, no setImmediate gap between session-delete and cache eviction. The race window that motivated Evict NetworkInterceptor map entries on MCP session deletion (HTTP transport) #763 (DELETE → immediate same-id reuse → stale NetworkInterceptor instance served) is now zero-width.
5. Composite-key compatible. Strips both <sessionId> and <sessionId>|<deviceId> keys; the device-scoped extension from chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762's fabe0ab1 keeps working unchanged.
6. Defense-in-depth on the eviction helper itself. Early return on empty sessionId, iterator-snapshot to avoid in-loop mutation hazards, explicit number return type for observability.

Merge Notes

• No file overlap with PR fix(ax-bridge): report inactive Flutter semantics #765 (fix/693-flutter-semantics-inactive-diagnostic) — independent merge order; either can land first.
• No file overlap with PR chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762 (release/0.6.1) for the net-new diff; the cache extraction touches files that chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762 also touched (src/tools/network-intercept.ts) but fix(network-intercept): evict session cache on HTTP delete #764 branches from develop after chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762's fabe0ab1 landed, so the merge resolves cleanly.
• Recommended order: chore(release): v0.6.1 — develop → main catch-up + AXSecureTextField paste fix (#761) #762 (release) → fix(network-intercept): evict session cache on HTTP delete #764 → fix(ax-bridge): report inactive Flutter semantics #765 (or fix(ax-bridge): report inactive Flutter semantics #765 → fix(network-intercept): evict session cache on HTTP delete #764). All three are independently mergeable today.

— /pr-review-os skill, senior-engineer pass.