From ced5d62b00b4df425c43f761d1d33ad5d21d595e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 22:15:35 +0800 Subject: [PATCH] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20R2=20=E2=80=94?= =?UTF-8?q?=20wenshao=20followup=20(visited=20set=20+=20dedup=20predicate)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Suggestions from wenshao's first PR #4411 review pass (07:15Z), both small and worth folding before merge: PR-A-R2 #1 (pid-descendants.ts:309 — walkDescendants visited set): `walkDescendants`'s BFS lacked a `visited` set. If the snapshot captures a PID-reuse cycle — rare but possible on busy hosts with rapid pid churn between `ps -A`'s start and parse, where Linux wraparound can show a freed pid in a different parent's children list creating an A→B / B→A cycle — pre-fix BFS would revisit nodes and fill the MAX_DESCENDANTS=256 quota with duplicate entries, starving legitimate descendants. Pre-PR-A the per-pid `pgrep` BFS had the same theoretical issue but was less exposed (each `pgrep -P pid` call returns only DIRECT children; snapshot captures the whole tree at once, making cycles instantly visible). Fix: 3-LOC `Set` add. `root` seeded into `visited` so a malformed snapshot listing root as a descendant of its own child doesn't re-enqueue root either. PR-A-R2 #2 (session-mcp-view.ts:117 — predicate dedup): After W12, the exported `passesSessionFilter` / `passesSessionPromptFilter` still called `passesNameFilter` (the pre-W12 array-based implementation), while `applyTools` / `applyPrompts` used `compiledFilterAccepts(compileNameFilter(...))`. Two parallel implementations of the same predicate — future change to one without the other would silently diverge: - the exported function's tests (passesSessionFilter unit tests) would still pass - the production filter path in applyTools/applyPrompts would behave differently Reviewer also noted `passesSessionPromptFilter` had zero callers in production code or tests after W12 — `applyPrompts` no longer references it. Kept the export rather than deleting it (matches the `passesSessionFilter` shape for symmetry + the F3 audit-path comment block earmarks both as the replay predicates), but routed both through `compiledFilterAccepts(compileNameFilter(...))` so there is a single source of truth. Set construction is per-call for these exports (negligible for unit-test / one-off probes); the bulk paths in `applyTools` / `applyPrompts` still construct ONE filter per pass via the original W12 code path. `passesNameFilter` (the standalone array-based helper) deleted — its only callers were the two exports, which now use the compiled path. Public-API surface unchanged: the two exported functions keep their signatures and semantics. Test sweep: 19/19 pid-descendants + session-mcp-view tests pass; typecheck + ESLint clean. Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) → 6cf18f641 (W12) → 2a41c6fae (R10) → this (R2 followups). --- packages/core/src/tools/pid-descendants.ts | 17 ++++++ packages/core/src/tools/session-mcp-view.ts | 63 +++++++++++---------- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index cbb789f60c..dae89f7936 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -305,9 +305,24 @@ async function listDescendantPidsWinPerPidFallback( * platforms' per-node subprocess forks once the snapshot has been * obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy * fallback path. Returns BFS order — children before grandchildren. + * + * F2 (#4175 commit 6 review fix — wenshao PR-A-R2 #1): `visited` + * set prevents BFS revisits when the snapshot captures a PID-reuse + * cycle (rare but possible on busy hosts with rapid pid churn + * between snapshot start and parse — Linux pid wraparound can make + * `ps -A` show a freed pid in a different parent's children list, + * producing an A→B / B→A cycle). Pre-fix the cycle would fill the + * MAX_DESCENDANTS=256 quota with duplicate entries and starve + * legitimate descendants. The per-pid `pgrep` BFS fallback had the + * same theoretical issue but was less exposed because each + * `pgrep -P pid` call only returns DIRECT children; the snapshot + * captures the whole tree at once. `root` is seeded into `visited` + * so a malformed snapshot listing root as a descendant of one of + * its own children doesn't re-enqueue root. */ function walkDescendants(tree: Map, root: number): number[] { const all: number[] = []; + const visited = new Set([root]); const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, ]; @@ -317,6 +332,8 @@ function walkDescendants(tree: Map, root: number): number[] { const children = tree.get(pid); if (!children) continue; for (const child of children) { + if (visited.has(child)) continue; + visited.add(child); if (all.length >= MAX_DESCENDANTS) break; all.push(child); queue.push({ pid: child, depth: depth + 1 }); diff --git a/packages/core/src/tools/session-mcp-view.ts b/packages/core/src/tools/session-mcp-view.ts index f720b033ff..c6b37535c2 100644 --- a/packages/core/src/tools/session-mcp-view.ts +++ b/packages/core/src/tools/session-mcp-view.ts @@ -13,35 +13,26 @@ import type { ToolRegistry } from './tool-registry.js'; const debugLogger = createDebugLogger('McpPool:View'); -function passesNameFilter( - name: string, - includeTools?: readonly string[], - excludeTools?: readonly string[], -): boolean { - if (excludeTools?.includes(name)) return false; - if (!includeTools) return true; - return includeTools.some((entry) => { - const stripped = entry.includes('(') - ? entry.slice(0, entry.indexOf('(')) - : entry; - return stripped === name; - }); -} - /** - * F2 (#4175 commit 6 review fix — wenshao W12 / PR A): precompute - * lookup `Set`s once per `applyTools` / `applyPrompts` pass so the - * per-tool predicate is O(1) instead of repeating the array scan - * inside `passesNameFilter` for every snapshot entry. Same semantics: - * `excludeTools` is direct-equality match (parens form not stripped — - * intentional pre-F2 behavior preserved); `includeTools` strips the - * first `(...)` suffix so `toolName(args)` matches `toolName`. + * F2 (#4175 commit 6 review fix — wenshao W12 / PR A; PR-A-R2 #2 + * folded the exports to delegate here): precompute lookup `Set`s + * once per `applyTools` / `applyPrompts` pass so the per-tool + * predicate is O(1) instead of repeating an array scan for every + * snapshot entry. Same semantics: `excludeTools` is direct-equality + * match (parens form not stripped — intentional pre-F2 behavior + * preserved); `includeTools` strips the first `(...)` suffix so + * `toolName(args)` matches `toolName`. * - * `passesSessionFilter` / `passesSessionPromptFilter` (the array- - * based predicates exported above) stay unchanged for unit tests - * and any caller that wants to test a single name without paying - * the Set-construction cost. The Sets live on `applyTools` / - * `applyPrompts`'s stack frame. + * PR-A-R2 #2: `passesSessionFilter` / `passesSessionPromptFilter` + * (exported below for unit-testability) now route THROUGH + * `compiledFilterAccepts(compileNameFilter(...))` so there is a + * single source of truth for the predicate. Pre-fix the exports + * called a separate `passesNameFilter` array-based implementation + * with the same semantics, creating a drift risk where a future + * change to one impl wouldn't be caught by tests of the other. + * The Set construction is per-call for these exports (cheap for + * tests / one-off probes); the bulk paths in + * `applyTools`/`applyPrompts` still construct ONE filter per pass. */ interface CompiledNameFilter { excludeSet?: ReadonlySet; @@ -91,13 +82,21 @@ function compiledFilterAccepts( * support, intentionally matching the existing pre-F2 behavior so * operators don't see semantic divergence between the two filter * lists when migrating sessions through pool mode. + * + * PR-A-R2 #2: routes through `compiledFilterAccepts(compileNameFilter(...))` + * so the bulk-path predicate and the exported per-name predicate + * share one implementation. Set construction is paid per call here + * (negligible for unit tests / one-off audit-path probes). */ export function passesSessionFilter( tool: DiscoveredMCPTool, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(tool.serverToolName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + tool.serverToolName, + ); } /** @@ -113,13 +112,19 @@ export function passesSessionFilter( * parens form `excludeTools: ['toolName(args)']` which only matches * tools (the parens-stripping in `passesSessionFilter` matches * `toolName` in the include list, not the exclude list). + * + * PR-A-R2 #2: same delegation to the compiled path as + * `passesSessionFilter`. */ export function passesSessionPromptFilter( promptName: string, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(promptName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + promptName, + ); } /**