feat(mcp): enforce the room-responder lock in room_post/alert_recipient (#42)#45
Conversation
…nt (#42) The PASSIVE role from the session bootstrap (#40) is model-followed instructions; a session that ignores them could still post as the agent through the IAK MCP tools. room_post and alert_recipient now check the responder lock before posting and refuse with the owner's identity + the takeover recipe when another live session holds it. Semantics mirror the bash hook exactly: lock path from IAK_RESPONDER_LOCK / <notification file>.responder.lock ('off' disables), owner identity = pid + process start time (recycled pids and dead owners read as stale -> allowed), fail open on mechanics. The lock-holding session is recognized by walking the caller's process-ancestor chain (the MCP server is spawned by the session process). Read-only room tools stay available to passive sessions. 9 new unit tests with an injectable fake process table; mcp-server suite 29/29. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
ThinkOffApp
left a comment
ThinkOffApp
left a comment
There was a problem hiding this comment.
Adversarial review: found one concrete availability issue in the ancestor walk. The PR otherwise covers the requested live-owner, pid-recycle, legacy-lock, and initial-ps-failure cases, but this intermediate ps failure path needs a fix and regression test before merge. GitHub would not allow REQUEST_CHANGES because the review identity is the PR author.
| return ''; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[P1] Preserve fail-open semantics when ancestor lookup fails. parentOf() collapses an exec('ps', ...) error into 0, which is indistinguishable from reaching the process root. If procStart() succeeds but ps -o ppid= fails for the current process or an intermediate wrapper, isSelfOrAncestor() returns false and this legitimate lock owner is refused, contrary to the documented “ps failures fail open” contract. Return a distinct unknown/error state and have assertRoomVoice() allow on that state, with a test where only the ancestor lookup throws.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42d6553346
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isSelfOrAncestor(lock.pid, selfPid, exec)) { | ||
| return { allowed: true, reason: 'this session holds the responder lock' }; |
There was a problem hiding this comment.
When Claude Code launches multiple logical sessions under the same parent pid, the bootstrap deliberately demotes a different session_id even if LOCK_PID == $PPID (checked scripts/session-bootstrap.sh:175-184, which notes sibling hook processes can share a parent). This new allow branch only checks that the lock pid is in the MCP server's ancestor chain and ignores lock.sid, so a PASSIVE sibling with the same parent pid is treated as the lock holder and can still use room_post/alert_recipient, bypassing the enforcement this commit adds.
Useful? React with 👍 / 👎.
Codex review on #45: an intermediate ps failure during the ancestor-chain walk collapsed into "chain ended" and could refuse a legitimate lock owner. parentOf now distinguishes the two cases via execFileSync's .status: nonzero exit = ps ran, pid gone = chain legitimately ended; no .status = ps never ran (ENOENT/EAGAIN) = unknown, propagated as null so assertRoomVoice fails OPEN. Two regression tests: spawn-level mid-walk failure -> allowed; exited walk ancestor -> chain ends -> refused. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed in the new commit. |
1 participant
Closes the client-side half of #42 (review finding 4 on #40): the PASSIVE role was model-followed instructions only — this makes the IAK toolpath refuse the post.
What
src/responder-lock.mjs: reads the lock written bysession-bootstrap.sh, evaluates owner liveness with the same pid + process-start-time identity as the hook, and decides whether the calling process may speak (assertRoomVoice). The owning session is recognized by finding the lock pid in the caller's ancestor chain (the MCP server is a child of the session process; walk capped at 10 levels for wrappers).room_postandalert_recipientrefuse with the owner's pid/session + takeover recipe when another live session holds the lock.room_recent(read-only) unaffected.Tests
9 new cases in
test/responder-lock.test.mjsusing an injectable fake process table: disabled/absent/unreadable locks, stale owner, recycled pid, owner-in-ancestor-chain allowed, live-foreign-owner refused (the #42 case), legacy no-pstart lock, ps-explosion fail-open.mcp-serversuite 29/29.Out of scope
Raw
curlagainst the GroupMind API can't be blocked client-side; the server-side fix (per-session keys) stays tracked in #42.🤖 Generated with Claude Code