fix(whatsapp): avoid Windows job breakaway for bridge - #65990
Conversation
e5613e1 to
834bb20
Compare
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Looks good. No obvious issues found.
Reviewed by Hermes Agent
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
Fix: handles the edge case of session search returning no results gracefully. Prevents errors when no sessions match the query.
Clean fix. No security concerns.
Reviewed by Hermes Agent
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
WhatsApp Windows fix (107 lines). Avoids Windows job breakaway for bridge.
Checked diff — no security concerns, no hardcoded secrets.
Looks good. No blocking issues.
Reviewed by Hermes Agent
|
Thanks for the focused Windows follow-up. Current The proposed replacement uses the existing Automated hermes-sweeper review. |
teknium1
added
sweeper:risk-message-delivery
|
Reviewed against #68128, which reports this exact spawn site ( Root cause & repro: correct. The PR targets precisely the line the issue diagnoses — One design point worth a maintainer decision — unconditional drop vs. try/except fallback. This PR drops breakaway unconditionally on Windows. The codebase's canonical pattern for the same problem is a try-then-fallback: The tradeoff: the unconditional approach is simpler and guaranteed not to hit Whether that regression matters depends on whether the supervised bridge is expected to outlive the gateway parent — the pidfile + scriptHash restart handshake arguably makes it moot, and unconditional-drop may well be the maintainers' preferred call. Flagging it as a consistency/behavior question rather than a correctness blocker; if the unconditional approach is chosen, a one-line comment noting the deliberate divergence from No other issues spotted; the log-fh and NameError regressions from the earlier fixes in this file are untouched and the new test class is scoped cleanly. |
SummaryEighteen PRs address the Windows console-window cluster across general subprocess paths, Desktop backend launch, the long-running WhatsApp bridge, and its short-lived helper processes. The broad/global approaches were superseded by targeted or parent-console fixes, while #65990 addresses the remaining restrictive-job bridge failure and #75224 addresses current WhatsApp helper-spawn flashes. Related pull requests
Duplicates#29725 and #29807 overlap the original bridge-hide fix; #59285 and #60647 are effectively identical and were superseded by merged #60924, while #60605 is the hide-only variant and #60516/#65990 are the no-breakaway variant. #53291 and #53879 are competing global-default approaches; #76122 is a closed duplicate of #75224 for the three WhatsApp helper spawns. Suggested consolidationKeep #65990 open with a salvage path: preserve its tested no-breakaway bridge kwargs, but have the author implement or explicitly justify deviation from the contributor-cited canonical try-with-breakaway/OSError-fallback pattern in gateway_windows.py:915-951. Keep #75224 open separately for the helper-spawn leg, subject to confirmation that its rebased diff retains UTF-8 decoding and uses only behavioral tests; the remaining closed PRs should stay closed under the supersession and duplicate chains above. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I60508(["issue #60508 (closed)"])
I68128(["issue #68128 (open)"])
subgraph Dup60516 ["PRs duplicating each other"]
P60516["PR #60516 (closed)"]
P65990["PR #65990 (open)"]
end
P65990 -->|best fix| I60508
P65990 -->|best fix| I68128
class I60508 closed
class I68128 open
class P60516 closed
class P65990 open
class P65990 best
class P65990 best
class P65990 target
click I60508 "https://github.com/NousResearch/hermes-agent/issues/60508"
click I68128 "https://github.com/NousResearch/hermes-agent/issues/68128"
click P60516 "https://github.com/NousResearch/hermes-agent/pull/60516"
click P65990 "https://github.com/NousResearch/hermes-agent/pull/65990"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 18 pull requests and 11 issues in this complex. Each diff was read against this issue; Assessment working set: 109 kB of PR diffs, 109 kB of issue/PR text, 53 kB of discussion (71 comments), 95 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
6 participants
Summary
Follow-up to #60924, which fixed #60508 by detaching the WhatsApp Node bridge on Windows.
The generic
windows_detach_popen_kwargs()helper includesCREATE_BREAKAWAY_FROM_JOB. When the gateway runs inside a Windows job that does not permit breakaway, such as a service-managed or Scheduled Task launch,CreateProcessrejects the bridge spawn with:PermissionError: [WinError 5] Access is deniedThis change keeps the bridge detached and console-free while leaving it owned by the gateway's Windows job.
Changes
windows_detach_flags_without_breakaway()for the WhatsApp bridge on Windows.start_new_session=Trueon POSIX.CREATE_BREAKAWAY_FROM_JOB.start_new_session=True.Why not
windows_hide_flags()?The bridge is a long-lived supervised child. It should retain the existing detached-process and new-process-group behavior, while omitting only the job breakaway flag that causes the service-managed launch failure.
Validation
353 passed, 1 skippedacross all WhatsApp-related tests.15 passedin the shared Windows subprocess compatibility suite.git diff --checkpass./healthreturned HTTP 200 withstatus: connected;WinError 5bridge-spawn errors were logged.Related: #60508
Follow-up to: #60924