Repository navigation
mcpx hooks (#2541 follow-up): bound descendant processes on timeout, cap retained output, count bus-level drops, real interpreter readiness — plus CI-only internal/hooks failure #2573
Description
Follow-up to #1872 / PR #2541 (merged 2026-10-30, merge commit 1aef00e). The PR merged with 4 Codex review findings undispositioned (review 2026-10-30T03:27:58Z by chatgpt-codex-connector[bot]) and a red basic: test run. Filing per the Codex-findings gate so none of them evaporate.
Codex findings to fix (all in the merged hooks implementation)
- P1 —
pkgs/mcpx/internal/hooks/runner.go:187Bound descendant processes and output-pipe waits.CommandContextkills only the direct child; descendants can hold stdout/stderr pipes open and keepcmd.Run()waiting for EOF indefinitely — so the hard timeout does not actually bound a hook that spawns children, contradicting mcpx: config-declared hooks over the event bus, running any command, JS/TS or Python module #1872'''s acceptance "a hanging hook cannot hang the daemon" for that case. Fix direction: process-group kill and/or bounded pipe draining. - P2 —
runner.go:86Account for event losses before per-hook fanout. The shared bus subscription can overflow before events reach per-hook queues;Subscription.Droppedis never read, soStats.Droppedunder-reports and the gap contract (Bump build/buck/preludes/buck2-prelude from3a02ea3to4371899#6) is not honoured for bus-level drops. - P2 —
runner.go:185Cap retained hook output. Both output streams accumulate unbounded in daemon memory, including successful runs whose output is discarded (ayeshook can allocate heavily inside the 2s default timeout). Cap and truncate. - P2 —
pkgs/mcpx/internal/cli/hook.go:93Check interpreter availability before marking a module ready. A declared Python module with neitherpython3noruvon PATH still reports ready inmcpx hook ls/ doctor, then fails on every matching event. Readiness must check the interpreter, not just the file.
CI-only internal/hooks failure
PR #2541'''s basic: test (run 40997046260, head eb4cabc) failed internal/hooks in 0.159s (the internal/defaults/internal/settings failures in the same run are the pre-existing doc-drift reds tracked in #2552). Reproduction attempt 2026-10-30 (queue task 3097, Lane D): go test ./internal/hooks/ -count=1 on the laptop at the exact merged commit eb4cabc passes 11/11 tests. The workflow log prints only package-level output (per-test JSON is filtered out), so the failing test is not identifiable from CI logs. Investigate: likely environment- or timing-sensitive (candidates: TestTimeoutCutsHangingHookAndPublishProceeds under runner load, or an executor-availability assumption). Consider surfacing per-test failures in the mcpx workflow log so this class is diagnosable.
Acceptance
- All 4 findings fixed with tests (descendant-spawning hanging hook is cut at timeout; bus drops counted; output capped; hook ls/doctor report unready when the interpreter is missing).
go test ./internal/hooks/...green in CI on the fixing PR.- Findings dispositioned on PR mcpx: config-declared hooks over the event bus (#1872) #2541 threads pointing here.
Acceptance criteria
- Timeout handling kills the hook's whole process group (or equivalently bounds pipe draining): a hook whose descendants hold stdout/stderr open cannot keep
cmd.Run()waiting past the timeout - Bus-level subscription drops are accounted:
Subscription.Droppedis read and reflected inStats.Dropped - Retained hook output is capped per run with truncation, for both streams, including successful runs
-
mcpx hook ls/ doctor readiness for a declared Python module checks interpreter availability (python3oruvon PATH), not just the file - The CI-only
internal/hooksfailure from PR mcpx: config-declared hooks over the event bus (#1872) #2541's run 40997046260 is identified (named test + mechanism) or per-test failure output is surfaced in the mcpx workflow so it becomes identifiable - Existing hooks tests pass; new regression tests pin the descendant-timeout and output-cap behaviour
Activity
dezren39 commentedon Oct 30, 2026
Intake review
Still live: follow-up to merged PR #2541 (#1872) — the four Codex findings are on code that is now on main, verified undispositioned at filing; a fix task is already queued (task 3823, enqueued by the lane that filed this). The CI-only
internal/hooksfailure is a fifth, separate thread in the same issue: it passes 11/11 on the laptop at the exact merged commit eb4cabc, so it is environment/timing-sensitive until proven otherwise.Related: #1872 (origin), PR #2541 (merged 1aef00e), #2552 (the pre-existing defaults/settings doc-drift reds in the same CI run — explicitly NOT part of this issue).
Notes / risks: finding 1 (descendant processes) is the P1: it negates the timeout guarantee #1872's acceptance promised. Findings 2–4 are P2 but each is small and localised in
internal/hooks/runner.go/internal/cli/hook.go. The CI-only failure should not block the four findings; if it cannot be reproduced, surfacing per-test JSON in the workflow is an acceptable resolution for that thread.Suggested approach: one PR for findings 1–4 with regression tests (process-group kill pattern already used elsewhere in the fleet), and a separate small look at the CI failure's timing sensitivity.
16 remaining items
fix(mcpx): exclude hook.* before buffering wildcard hook subscriptions (
dezren39 commentedon Oct 30, 2026
🤖 Agent note · parked
chatgpt-codex-connector commentedon Oct 30, 2026
To use Codex here, create an environment for this repo.
dezren39 commentedon Oct 31, 2026
🤖 Agent note · picked up
queue/3823-2573-mcpx-hooksMerge pull request #2840 from developing-today/queue/3823-2573-mcpx-h…
dezren39 commentedon Oct 31, 2026
🤖 Agent note · done
b5d334b0b6de7328dc8188f90aadf90fd5c1ae14.8db2e07bclean ("Didn't find any major issues", 2026-10-30T23:41:58Z) and all 4 original threads on mcpx: config-declared hooks over the event bus (#1872) #2541 now carry a reply pointing at the fix and are resolved. The 3 CodeRabbit threads on fix(mcpx): disposition the 4 undispositioned Codex findings from PR #2541 (#2573) #2840 are dispositioned: the wildcard drop-count finding was already fixed byHook.SubFilter()in8db2e07b; the 2 remaining valid minors (streaming CI log formatter; separate stderr cap for hook failure text) are tracked in mcpx: CodeRabbit minor follow-ups from PR #2840 — stream the CI log formatter, separate stderr cap for hook failure text #2880. CI: lint + plugin tests + mcpx-docs pass; the stress leg's only failure is the pre-existing mcpx: internal/cli package test duration exceeds the 600s stress timeout on PR 2714 (stage 1 of #1980) #2859internal/clitimeout (TestEverySettingFlagReachesTheResolvedValue) —internal/hookspassed the same leg (ok 22.797s, -count=10 race/shuffle). The one-off CI-onlyinternal/hooksfailure from mcpx: config-declared hooks over the event bus (#1872) #2541 headeb4cabc6has not recurred across any fix(mcpx): disposition the 4 undispositioned Codex findings from PR #2541 (#2573) #2840 run.